public inbox for [email protected]
help / color / mirror / Atom feedRe: Add semi-join pushdown to postgres_fdw
2+ messages / 2 participants
[nested] [flat]
* Re: Add semi-join pushdown to postgres_fdw
@ 2023-12-05 10:29 Alexander Pyhalov <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Alexander Pyhalov @ 2023-12-05 10:29 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Tomas Vondra <[email protected]>; [email protected] <[email protected]>; Ashutosh Bapat <[email protected]>; pgsql-hackers; Ian Lawrence Barwick <[email protected]>
Alexander Korotkov писал(а) 2023-12-03 23:52:
> Hi, Alexander!
>
> On Mon, Nov 27, 2023 at 5:11 PM Alexander Pyhalov
> <[email protected]> wrote:
>> Alexander Korotkov писал(а) 2023-11-27 03:49:
>>
>> > Thank you for the revision.
>> >
>> > I've revised the patch myself. I've replaced StringInfo with
>> > additional conds into a list of strings as I proposed before. I think
>> > the code became much clearer. Also, it gets rid of some unnecessary
>> > allocations.
>> >
>> > I think the code itself is not in bad shape. But patch lacks some
>> > high-level description of semi-joins processing as well as comments on
>> > each manipulation with additional conds. Could you please add this?
>> >
>>
>> Hi. The updated patch looks better. It seems I've failed to fix logic
>> in
>> deparseFromExprForRel() when tried to convert StringInfos to Lists.
>>
>> I've added some comments. The most complete description of how
>> SEMI-JOIN
>> is processed, is located in deparseFromExprForRel(). Unfortunately,
>> there seems to be no single place, describing current JOIN deparsing
>> logic.
>
> Looks good to me. I've made some grammar and formatting adjustments.
> Also, I've written the commit message.
>
> Now, I think this looks good. I'm going to push this if no objections.
>
> ------
> Regards,
> Alexander Korotkov
Hi. No objections from my side.
Perhaps, some rephrasing is needed in comment in semijoin_target_ok():
"The planner can create semi-joins, which refer to inner rel
vars in its target list."
Perhaps, change "semi-joins, which refer" to "a semi-join, which refers
...",
as later we speak about "its" target list.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
^ permalink raw reply [nested|flat] 2+ messages in thread
* [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v11-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch"
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2024-02-16 01:13 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-12-05 10:29 Re: Add semi-join pushdown to postgres_fdw Alexander Pyhalov <[email protected]>
2024-02-16 01:13 [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox