public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact 2+ messages / 2 participants [nested] [flat]
* [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact @ 2024-02-27 01:34 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw) Future commits will remove the TBMIterateResult from BitmapHeapNext() -- pushing it into the table AM-specific code. So, the table AM must inform BitmapHeapNext() whether or not the current block is lossy or exact for the purposes of the counters used in EXPLAIN. --- src/backend/access/heap/heapam_handler.c | 5 ++++- src/backend/executor/nodeBitmapHeapscan.c | 10 +++++----- src/include/access/tableam.h | 14 ++++++++++---- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 7fdccaf613..849cac3947 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths, static bool heapam_scan_bitmap_next_block(TableScanDesc scan, - TBMIterateResult *tbmres) + TBMIterateResult *tbmres, + bool *lossy) { HeapScanDesc hscan = (HeapScanDesc) scan; BlockNumber block = tbmres->blockno; @@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, Assert(ntup <= MaxHeapTuplesPerPage); hscan->rs_ntuples = ntup; + *lossy = tbmres->ntuples < 0; + return ntup > 0; } diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 404de0595e..c95e3412da 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node) for (;;) { - bool valid; + bool valid, lossy; CHECK_FOR_INTERRUPTS(); @@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node) BitmapAdjustPrefetchIterator(node, tbmres->blockno); - valid = table_scan_bitmap_next_block(scan, tbmres); + valid = table_scan_bitmap_next_block(scan, tbmres, &lossy); - if (tbmres->ntuples >= 0) - node->exact_pages++; - else + if (lossy) node->lossy_pages++; + else + node->exact_pages++; if (!valid) { diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 1bc5f7c057..b9ba4f9fb3 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -804,6 +804,9 @@ typedef struct TableAmRoutine * on the page have to be returned, otherwise the tuples at offsets in * `tbmres->offsets` need to be returned. * + * lossy indicates whether or not the block's representation in the bitmap + * is lossy or exact. + * * XXX: Currently this may only be implemented if the AM uses md.c as its * storage manager, and uses ItemPointer->ip_blkid in a manner that maps * blockids directly to the underlying storage. nodeBitmapHeapscan.c @@ -819,7 +822,8 @@ typedef struct TableAmRoutine * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_block) (TableScanDesc scan, - struct TBMIterateResult *tbmres); + struct TBMIterateResult *tbmres, + bool *lossy); /* * Fetch the next tuple of a bitmap table scan into `slot` and return true @@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths, * Prepare to fetch / check / return tuples from `tbmres->blockno` as part of * a bitmap table scan. `scan` needs to have been started via * table_beginscan_bm(). Returns false if there are no tuples to be found on - * the page, true otherwise. + * the page, true otherwise. lossy is set to true if bitmap is lossy for the + * selected block and false otherwise. * * Note, this is an optionally implemented function, therefore should only be * used after verifying the presence (at plan time or such). */ static inline bool table_scan_bitmap_next_block(TableScanDesc scan, - struct TBMIterateResult *tbmres) + struct TBMIterateResult *tbmres, + bool *lossy) { /* * We don't expect direct calls to table_scan_bitmap_next_block with valid @@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan, - tbmres); + tbmres, lossy); } /* -- 2.40.1 --xqq4defy3uncu6k6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition @ 2025-02-13 14:41 vignesh C <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: vignesh C @ 2025-02-13 14:41 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Thu, 13 Feb 2025 at 15:50, Shlok Kyal <[email protected]> wrote: > > > I have fixed the issue. Attached the updated v6 patch. There is another concurrency issue: In case of create publication for all tables with publish_via_partition_root we will call check_foreign_tables: @@ -876,6 +876,10 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) /* Associate objects with the publication. */ if (stmt->for_all_tables) { + /* Check if any foreign table is a part of partitioned table */ + if (publish_via_partition_root) + check_foreign_tables(stmt->pubname); At the time of check in check_foreign_tables, there are no foreign tables so this check will be successful: +check_foreign_tables_in_schema(Oid schemaid, char *pubname) +{ + Relation classRel; + ScanKeyData key[2]; + TableScanDesc scan; + HeapTuple tuple; + + classRel = table_open(RelationRelationId, AccessShareLock); + + ScanKeyInit(&key[0], + Anum_pg_class_relnamespace, + BTEqualStrategyNumber, F_OIDEQ, + schemaid); + ScanKeyInit(&key[1], + Anum_pg_class_relkind, + BTEqualStrategyNumber, F_CHAREQ, + CharGetDatum(RELKIND_PARTITIONED_TABLE)); + + scan = table_beginscan_catalog(classRel, 2, key); + while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) Now immediately after execution of this, create a foreign table: postgres=# CREATE FOREIGN TABLE part22 PARTITION OF part2 FOR VALUES FROM (10) TO (15) SERVER fdw; CREATE FOREIGN TABLE And then continue execution of create publication, it will also be successful: postgres=# create publication pub1 for all tables with ( publish_via_partition_root =true); CREATE PUBLICATION One probable way to fix this is to do the search similar to check_foreign_tables_in_schema where we can skip including schemaid key for all tables. How about something like the attached patch. Regards, Vignesh Attachments: [text/x-patch] Concurrency_all_tables_issue.patch (3.8K, ../../CALDaNm3bULhtCaaBmdsun0LnHx9Jp0v228kjd=ZujEaaCEOmAA@mail.gmail.com/2-Concurrency_all_tables_issue.patch) download | inline diff: diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 520fa2f382..3214104dec 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -1388,21 +1388,24 @@ check_foreign_tables_in_schema(Oid schemaid, char *pubname) { Relation classRel; ScanKeyData key[2]; + int keycount = 0; TableScanDesc scan; HeapTuple tuple; classRel = table_open(RelationRelationId, AccessShareLock); - ScanKeyInit(&key[0], - Anum_pg_class_relnamespace, - BTEqualStrategyNumber, F_OIDEQ, - schemaid); - ScanKeyInit(&key[1], + ScanKeyInit(&key[keycount++], Anum_pg_class_relkind, BTEqualStrategyNumber, F_CHAREQ, CharGetDatum(RELKIND_PARTITIONED_TABLE)); - - scan = table_beginscan_catalog(classRel, 2, key); + + if (OidIsValid(schemaid)) + ScanKeyInit(&key[keycount++], + Anum_pg_class_relnamespace, + BTEqualStrategyNumber, F_OIDEQ, + schemaid); + + scan = table_beginscan_catalog(classRel, keycount, key); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple); @@ -1436,46 +1439,3 @@ check_foreign_tables_in_schema(Oid schemaid, char *pubname) table_endscan(scan); table_close(classRel, AccessShareLock); } - -/* Check if any foreign table is a partition table */ -void -check_foreign_tables(char *pubname) -{ - Relation classRel; - ScanKeyData key[1]; - TableScanDesc scan; - HeapTuple tuple; - - classRel = table_open(RelationRelationId, AccessShareLock); - - ScanKeyInit(&key[0], - Anum_pg_class_relkind, - BTEqualStrategyNumber, F_CHAREQ, - CharGetDatum(RELKIND_FOREIGN_TABLE)); - - scan = table_beginscan_catalog(classRel, 1, key); - while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) - { - Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple); - - if (relForm->relispartition) - { - Oid parent_oid; - char *parent_name; - List *ancestors = get_partition_ancestors(relForm->oid); - - parent_oid = llast_oid(ancestors); - parent_name = get_rel_name(parent_oid); - - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("cannot set parameter \"%s\" to true for publication \"%s\"", - "publish_via_partition_root", pubname), - errdetail("partition table \"%s\" in publication contains a foreign partition", - parent_name))); - } - } - - table_endscan(scan); - table_close(classRel, AccessShareLock); -} diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index e71d5408c7..b8da4ed130 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -878,7 +878,7 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) { /* Check if any foreign table is a part of partitioned table */ if (publish_via_partition_root) - check_foreign_tables(stmt->pubname); + check_foreign_tables_in_schema(InvalidOid, stmt->pubname); /* Invalidate relcache so that publication info is rebuilt. */ CacheInvalidateRelcacheAll(); @@ -1057,7 +1057,7 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, char *pubname = stmt->pubname; if (pubform->puballtables) - check_foreign_tables(pubname); + check_foreign_tables_in_schema(InvalidOid, pubname); schemaoids = GetPublicationSchemas(pubform->oid); diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index b1bb864d2f..fcd428465e 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -196,6 +196,4 @@ extern bool check_partrel_has_foreign_table(Form_pg_class relform); extern void check_foreign_tables_in_schema(Oid schemaid, char *pubname); -extern void check_foreign_tables(char *pubname); - #endif /* PG_PUBLICATION_H */ ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2025-02-13 14:41 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-02-27 01:34 [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]> 2025-02-13 14:41 Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[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