public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH 3/6] Be less verbose on variable names
34+ messages / 9 participants
[nested] [flat]

* [PATCH 3/6] Be less verbose on variable names
@ 2019-11-28 22:30  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Alvaro Herrera @ 2019-11-28 22:30 UTC (permalink / raw)

---
 src/backend/commands/typecmds.c            | 23 +++++++++-------------
 src/backend/utils/adt/pg_upgrade_support.c |  4 ++--
 src/include/catalog/binary_upgrade.h       |  4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 1b012c9cad..4ca2d3364b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -85,8 +85,8 @@ typedef struct
 
 /* Potentially set by pg_upgrade_support functions */
 Oid			binary_upgrade_next_array_pg_type_oid = InvalidOid;
-Oid			binary_upgrade_next_multirange_pg_type_oid = InvalidOid;
-Oid			binary_upgrade_next_multirange_array_pg_type_oid = InvalidOid;
+Oid			binary_upgrade_next_mrng_pg_type_oid = InvalidOid;
+Oid			binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid;
 
 static void makeRangeConstructors(const char *name, Oid namespace,
 								  Oid rangeOid, Oid subtype);
@@ -1529,13 +1529,9 @@ DefineRange(CreateRangeStmt *stmt)
 	/* alignment must be 'i' or 'd' for ranges */
 	alignment = (subtypalign == 'd') ? 'd' : 'i';
 
-	/* Allocate OID for array type */
+	/* Allocate OID for array type, its multirange and its multirange array */
 	rangeArrayOid = AssignTypeArrayOid();
-
-	/* Allocate OID for multirange type */
 	multirangeOid = AssignTypeMultirangeOid();
-
-	/* Allocate OID for multirange array type */
 	multirangeArrayOid = AssignTypeMultirangeArrayOid();
 
 	/* Create the pg_type entry */
@@ -1574,7 +1570,6 @@ DefineRange(CreateRangeStmt *stmt)
 	Assert(typoid == address.objectId);
 
 	/* Create the multirange that goes with it */
-
 	multirangeTypeName = makeMultirangeTypeName(typeName, typeNamespace);
 
 	mltrngaddress =
@@ -2313,13 +2308,13 @@ AssignTypeMultirangeOid(void)
 	/* Use binary-upgrade override for pg_type.oid? */
 	if (IsBinaryUpgrade)
 	{
-		if (!OidIsValid(binary_upgrade_next_multirange_pg_type_oid))
+		if (!OidIsValid(binary_upgrade_next_mrng_pg_type_oid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("pg_type multirange OID value not set when in binary upgrade mode")));
 
-		type_multirange_oid = binary_upgrade_next_multirange_pg_type_oid;
-		binary_upgrade_next_multirange_pg_type_oid = InvalidOid;
+		type_multirange_oid = binary_upgrade_next_mrng_pg_type_oid;
+		binary_upgrade_next_mrng_pg_type_oid = InvalidOid;
 	}
 	else
 	{
@@ -2346,13 +2341,13 @@ AssignTypeMultirangeArrayOid(void)
 	/* Use binary-upgrade override for pg_type.oid? */
 	if (IsBinaryUpgrade)
 	{
-		if (!OidIsValid(binary_upgrade_next_multirange_array_pg_type_oid))
+		if (!OidIsValid(binary_upgrade_next_mrng_array_pg_type_oid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("pg_type multirange array OID value not set when in binary upgrade mode")));
 
-		type_multirange_array_oid = binary_upgrade_next_multirange_array_pg_type_oid;
-		binary_upgrade_next_multirange_array_pg_type_oid = InvalidOid;
+		type_multirange_array_oid = binary_upgrade_next_mrng_array_pg_type_oid;
+		binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid;
 	}
 	else
 	{
diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c
index d980b96f48..418c26c81b 100644
--- a/src/backend/utils/adt/pg_upgrade_support.c
+++ b/src/backend/utils/adt/pg_upgrade_support.c
@@ -57,7 +57,7 @@ binary_upgrade_set_next_multirange_pg_type_oid(PG_FUNCTION_ARGS)
 	Oid			typoid = PG_GETARG_OID(0);
 
 	CHECK_IS_BINARY_UPGRADE;
-	binary_upgrade_next_multirange_pg_type_oid = typoid;
+	binary_upgrade_next_mrng_pg_type_oid = typoid;
 
 	PG_RETURN_VOID();
 }
@@ -68,7 +68,7 @@ binary_upgrade_set_next_multirange_array_pg_type_oid(PG_FUNCTION_ARGS)
 	Oid			typoid = PG_GETARG_OID(0);
 
 	CHECK_IS_BINARY_UPGRADE;
-	binary_upgrade_next_multirange_array_pg_type_oid = typoid;
+	binary_upgrade_next_mrng_array_pg_type_oid = typoid;
 
 	PG_RETURN_VOID();
 }
diff --git a/src/include/catalog/binary_upgrade.h b/src/include/catalog/binary_upgrade.h
index 2b6e87bb84..ba132ddf23 100644
--- a/src/include/catalog/binary_upgrade.h
+++ b/src/include/catalog/binary_upgrade.h
@@ -16,8 +16,8 @@
 
 extern PGDLLIMPORT Oid binary_upgrade_next_pg_type_oid;
 extern PGDLLIMPORT Oid binary_upgrade_next_array_pg_type_oid;
-extern PGDLLIMPORT Oid binary_upgrade_next_multirange_pg_type_oid;
-extern PGDLLIMPORT Oid binary_upgrade_next_multirange_array_pg_type_oid;
+extern PGDLLIMPORT Oid binary_upgrade_next_mrng_pg_type_oid;
+extern PGDLLIMPORT Oid binary_upgrade_next_mrng_array_pg_type_oid;
 extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_type_oid;
 
 extern PGDLLIMPORT Oid binary_upgrade_next_heap_pg_class_oid;
-- 
2.20.1


--+QahgC5+KEYLbs62
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0004-Protect-comment-against-pgindent.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v8-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v10-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index ce47a158ae..ddcdbbaf7e 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2308,7 +2308,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 5c2a7b7422..9c7b8bf162 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -806,10 +806,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -840,15 +837,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2060,7 +2052,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2072,7 +2063,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v11-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 10c1c3b616b..a1ec50ab7a8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 248ff90904b..04ad14f70b3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -287,7 +287,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f1d0d4b78e3..e35bd36e710 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -787,10 +787,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -821,15 +818,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v6-0010-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 10c1c3b616b..a1ec50ab7a8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 94a5e2da17c..bbdaa591891 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f1d0d4b78e3..e35bd36e710 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -787,10 +787,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -821,15 +818,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v8-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v10-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 10c1c3b616b..a1ec50ab7a8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 248ff90904b..04ad14f70b3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -287,7 +287,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f1d0d4b78e3..e35bd36e710 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -787,10 +787,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -821,15 +818,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v6-0010-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 10c1c3b616b..a1ec50ab7a8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 94a5e2da17c..bbdaa591891 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f1d0d4b78e3..e35bd36e710 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -787,10 +787,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -821,15 +818,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v8-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v10-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index ce47a158ae..ddcdbbaf7e 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2308,7 +2308,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 5c2a7b7422..9c7b8bf162 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -806,10 +806,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -840,15 +837,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2060,7 +2052,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2072,7 +2063,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v11-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v12 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 973734e9ffa..2cb5fb18675 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2335,7 +2335,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed41..282dcb97919 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 19e99c81092..ff3e8bcdd6f 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -802,10 +802,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -836,15 +833,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -2050,7 +2042,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2062,7 +2053,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--6jpz2j246qmht4bt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v12-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 10c1c3b616b..a1ec50ab7a8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 248ff90904b..04ad14f70b3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -287,7 +287,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f1d0d4b78e3..e35bd36e710 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -787,10 +787,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -821,15 +818,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v6-0010-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)

With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.

Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().

Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
 src/backend/access/heap/heapam_handler.c  |  1 -
 src/backend/executor/nodeBitmapHeapscan.c |  2 +-
 src/include/access/tableam.h              | 12 +-----------
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 10c1c3b616b..a1ec50ab7a8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TBMIterateResult *tbmres,
 							  TupleTableSlot *slot)
 {
 	HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 94a5e2da17c..bbdaa591891 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		/*
 		 * Attempt to fetch tuple from AM.
 		 */
-		if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+		if (!table_scan_bitmap_next_tuple(scan, slot))
 		{
 			/* nothing more to look at on this page */
 			node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index f1d0d4b78e3..e35bd36e710 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -787,10 +787,7 @@ typedef struct TableAmRoutine
 	 *
 	 * This will typically read and pin the target block, and do the necessary
 	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time). For some
-	 * AMs it will make more sense to do all the work referencing `tbmres`
-	 * contents here, for others it might be better to defer more work to
-	 * scan_bitmap_next_tuple.
+	 * make sense to perform tuple visibility checks at this time).
 	 *
 	 * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
 	 * on the page have to be returned, otherwise the tuples at offsets in
@@ -821,15 +818,10 @@ typedef struct TableAmRoutine
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * For some AMs it will make more sense to do all the work referencing
-	 * `tbmres` contents in scan_bitmap_next_block, for others it might be
-	 * better to defer more work to this callback.
-	 *
 	 * Optional callback, but either both scan_bitmap_next_block and
 	 * scan_bitmap_next_tuple need to exist, or neither.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   struct TBMIterateResult *tbmres,
 										   TupleTableSlot *slot);
 
 	/*
@@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 struct TBMIterateResult *tbmres,
 							 TupleTableSlot *slot)
 {
 	/*
@@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   tbmres,
 														   slot);
 }
 
-- 
2.40.1


--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2024-12-05 09:43  Jakub Wartak <[email protected]>
  0 siblings, 2 replies; 34+ messages in thread

From: Jakub Wartak @ 2024-12-05 09:43 UTC (permalink / raw)
  To: pgsql-hackers

One of our customers ran into a very odd case, where hot standby feedback
backend_xmin propagation stopped working due to major (hours/days) clock
time shifts on hypervisor-managed VMs. This happens (and is fully
reproducible) e.g. in scenarios where standby connects and its own VM is
having time from the future (relative to primary) and then that time goes
back to "normal". In such situation "sends hot_standby_feedback xmin"
timestamp messages are stopped being transferred, e.g.:

2024-12-05 02:02:35 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot
standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending
write 6/E9015230 flush 6/E9015230 apply 6/E9015230
2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot
standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
<-- clock readjustment and no further "sending hot standby feedback"
2024-12-04 14:18:54 UTC [6002]: db=,user=,app=,client= DEBUG:  sendtime
2024-12-04 14:18:51.836936+00 receipttime 2024-12-04 14:18:54.199223+00
replication apply delay 0 ms transfer latency 2363 ms
2024-12-04 14:18:54 UTC [6002]: db=,user=,app=,client= DEBUG:  sending
write 6/E9015258 flush 6/E9015230 apply 6/E9015230
2024-12-04 14:18:54 UTC [6002]: db=,user=,app=,client= DEBUG:  sending
write 6/E9015258 flush 6/E9015258 apply 6/E9015258
2024-12-04 14:18:54 UTC [6002]: db=,user=,app=,client= DEBUG:  sending
write 6/E9015258 flush 6/E9015258 apply 6/E9015258
2024-12-04 14:18:55 UTC [6002]: db=,user=,app=,client= DEBUG:  sendtime
2024-12-04 14:18:53.136738+00 receipttime 2024-12-04 14:18:55.498946+00
replication apply delay 0 ms transfer latency 2363 ms
2024-12-04 14:18:55 UTC [6002]: db=,user=,app=,client= DEBUG:  sending
write 6/E9015280 flush 6/E9015258 apply 6/E9015258
2024-12-04 14:18:55 UTC [6002]: db=,user=,app=,client= DEBUG:  sending
write 6/E9015280 flush 6/E9015280 apply 6/E9015280

I can share reproduction steps if anyone is interested. This basically
happens due to usage of TimestampDifferenceExceeds() in
XLogWalRcvSendHSFeedback(), but I bet there are other similiar scenarios.

What I was kind of surprised about was the lack of recommendation for
having primary/standby to have clocks synced when using
hot_standby_feedback, but such a thing is mentioned for
recovery_min_apply_delay. So I would like to add at least one sentence to
hot_standby_feedback to warn about this too, patch attached.

-J.


Attachments:

  [application/octet-stream] v1-0001-doc-Mention-clock-synchronization-recommendation-.patch (1.2K, ../../CAKZiRmwBcALLrDgCyEhHP1enUxtPMjyNM_d1A2Lng3_6Rf4Qfw@mail.gmail.com/3-v1-0001-doc-Mention-clock-synchronization-recommendation-.patch)
  download | inline diff:
From 56480c6fb9b5bd62f2fa3a3c9235c41cb57115eb Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 5 Dec 2024 10:35:58 +0100
Subject: [PATCH v1] doc: Mention clock synchronization recommendation for
 hot_standby_feedback

hot_standby_feedback mechanics assume that clocks are synchronized, but it
was clear from documentation.
---
 doc/src/sgml/config.sgml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e0c8325a39..caf87be60e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5038,6 +5038,13 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
         until it eventually reaches the primary.  Standbys make no other use
         of feedback they receive other than to pass upstream.
        </para>
+       <para>
+        Using this option requires the primary and standby(s) to have system
+        clocks synchronized, otherwise it may lead to prolonged risk of not
+        removing dead rows on primary for extended periods of time as the
+        feedback mechanism is based on timestamps exchanged between primary
+        and standby(s).
+       </para>
       </listitem>
      </varlistentry>
 
-- 
2.39.5



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2024-12-05 15:06  Euler Taveira <[email protected]>
  parent: Jakub Wartak <[email protected]>
  1 sibling, 2 replies; 34+ messages in thread

From: Euler Taveira @ 2024-12-05 15:06 UTC (permalink / raw)
  To: Jakub Wartak <[email protected]>; pgsql-hackers

On Thu, Dec 5, 2024, at 6:43 AM, Jakub Wartak wrote:
> One of our customers ran into a very odd case, where hot standby feedback backend_xmin propagation stopped working due to major (hours/days) clock time shifts on hypervisor-managed VMs. This happens (and is fully reproducible) e.g. in scenarios where standby connects and its own VM is having time from the future (relative to primary) and then that time goes back to "normal". In such situation "sends hot_standby_feedback xmin" timestamp messages are stopped being transferred, e.g.:

Is it worth a WARNING message if there is a "huge" time difference
between the servers? We already have the reply time in the message so
it is a matter of defining the "huge" interval plus a roundtrip. We also
need to avoid spamming the log.

Your patch looks good to me. Should it be converted into a
<note>...</note>? (See synchronous_standby_names [1] for an example.)

[1] https://www.postgresql.org/docs/current/runtime-config-replication.html


--
Euler Taveira
EDB   https://www.enterprisedb.com/


^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2024-12-05 15:17  Andres Freund <[email protected]>
  parent: Euler Taveira <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Andres Freund @ 2024-12-05 15:17 UTC (permalink / raw)
  To: Euler Taveira <[email protected]>; +Cc: Jakub Wartak <[email protected]>; pgsql-hackers

On 2024-12-05 12:06:41 -0300, Euler Taveira wrote:
> Is it worth a WARNING message if there is a "huge" time difference
> between the servers? We already have the reply time in the message so
> it is a matter of defining the "huge" interval plus a roundtrip. We also
> need to avoid spamming the log.

IME folks who have huge time differences between the servers are not going to
look at the log carefully enough to see such a warning.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2024-12-09 10:52  Jakub Wartak <[email protected]>
  parent: Euler Taveira <[email protected]>
  1 sibling, 0 replies; 34+ messages in thread

From: Jakub Wartak @ 2024-12-09 10:52 UTC (permalink / raw)
  To: Euler Taveira <[email protected]>; +Cc: pgsql-hackers

Hi Euler!,

On Thu, Dec 5, 2024 at 4:07 PM Euler Taveira <[email protected]> wrote:

> On Thu, Dec 5, 2024, at 6:43 AM, Jakub Wartak wrote:
>
> One of our customers ran into a very odd case, where hot standby feedback
> backend_xmin propagation stopped working due to major (hours/days) clock
> time shifts on hypervisor-managed VMs. This happens (and is fully
> reproducible) e.g. in scenarios where standby connects and its own VM is
> having time from the future (relative to primary) and then that time goes
> back to "normal". In such situation "sends hot_standby_feedback xmin"
> timestamp messages are stopped being transferred, e.g.:
>
>
> Is it worth a WARNING message if there is a "huge" time difference
> between the servers? We already have the reply time in the message so
> it is a matter of defining the "huge" interval plus a roundtrip. We also
> need to avoid spamming the log.
>

I'm trying to stay consistent with what the recovery_min_apply_delay did
(there is a warning in the docs, but there's no warning in code) and I just
wanted the to have pointer in the documentation that if someone is using
hot_standby_feedback then he would be at least warned before. Given it is
very rare I don't think we need additional code (+ what Andres has noted ).


> Your patch looks good to me. Should it be converted into a
> <note>...</note>? (See synchronous_standby_names [1] for an example.)
>

Fine for me, but we would have to also convert the recovery_min_apply_delay
to do the same, right?

-J.


^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2024-12-18 09:33  Amit Kapila <[email protected]>
  parent: Jakub Wartak <[email protected]>
  1 sibling, 1 reply; 34+ messages in thread

From: Amit Kapila @ 2024-12-18 09:33 UTC (permalink / raw)
  To: Jakub Wartak <[email protected]>; +Cc: pgsql-hackers

On Thu, Dec 5, 2024 at 3:14 PM Jakub Wartak
<[email protected]> wrote:
>
> One of our customers ran into a very odd case, where hot standby feedback backend_xmin propagation stopped working due to major (hours/days) clock time shifts on hypervisor-managed VMs. This happens (and is fully reproducible) e.g. in scenarios where standby connects and its own VM is having time from the future (relative to primary) and then that time goes back to "normal". In such situation "sends hot_standby_feedback xmin" timestamp messages are stopped being transferred, e.g.:
>
> 2024-12-05 02:02:35 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
> 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending write 6/E9015230 flush 6/E9015230 apply 6/E9015230
> 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
> <-- clock readjustment and no further "sending hot standby feedback"
...
>
> I can share reproduction steps if anyone is interested. This basically happens due to usage of TimestampDifferenceExceeds() in XLogWalRcvSendHSFeedback(), but I bet there are other similiar scenarios.
>

We started to use a different mechanism in HEAD. See XLogWalRcvSendHSFeedback().

> What I was kind of surprised about was the lack of recommendation for having primary/standby to have clocks synced when using hot_standby_feedback, but such a thing is mentioned for recovery_min_apply_delay. So I would like to add at least one sentence to hot_standby_feedback to warn about this too, patch attached.
>

IIUC, this issue doesn't occur because the primary and standby clocks
are not synchronized. It happened because the clock on standby moved
backward. This is quite unlike the 'recovery_min_apply_delay' where
non-synchronization of clocks between primary and standby can lead to
unexpected results. This is because we don't compare any time on the
primary with the time on standby. If this understanding is correct
then the wording proposed by your patch should be changed accordingly.

-- 
With Regards,
Amit Kapila.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-01-08 12:49  Jakub Wartak <[email protected]>
  parent: Amit Kapila <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Jakub Wartak @ 2025-01-08 12:49 UTC (permalink / raw)
  To: Amit Kapila <[email protected]>; +Cc: pgsql-hackers

On Wed, Dec 18, 2024 at 10:33 AM Amit Kapila <[email protected]> wrote:

Hi Amit!

> On Thu, Dec 5, 2024 at 3:14 PM Jakub Wartak
> <[email protected]> wrote:
> >
> > One of our customers ran into a very odd case, where hot standby feedback backend_xmin propagation stopped working due to major (hours/days) clock time shifts on hypervisor-managed VMs. This happens (and is fully reproducible) e.g. in scenarios where standby connects and its own VM is having time from the future (relative to primary) and then that time goes back to "normal". In such situation "sends hot_standby_feedback xmin" timestamp messages are stopped being transferred, e.g.:
> >
> > 2024-12-05 02:02:35 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
> > 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending write 6/E9015230 flush 6/E9015230 apply 6/E9015230
> > 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
> > <-- clock readjustment and no further "sending hot standby feedback"
> ...
> >
> > I can share reproduction steps if anyone is interested. This basically happens due to usage of TimestampDifferenceExceeds() in XLogWalRcvSendHSFeedback(), but I bet there are other similiar scenarios.
> >
>
> We started to use a different mechanism in HEAD. See XLogWalRcvSendHSFeedback().

Yes, you are correct somewhat because I was looking on REL13_STABLE,
but I've taken a fresh quick look at 05a7be93558 and tested it too.
Sadly, PG17 still maintains the same behavior of lack of proper
backend_xmin propagation (it stops sending hot standby feedback once
time on standby jumps forward). I believe this is the case because
walreceiver schedules next wakeup in far far future (when the clock /
now() is way ahead, see WalRcvComputeNextWakeup()), so when the clock
is back to normal (resetted back -X hours/days), the next wakeup seems
to be +X hours/days ahead.

> > What I was kind of surprised about was the lack of recommendation for having primary/standby to have clocks synced when using hot_standby_feedback, but such a thing is mentioned for recovery_min_apply_delay. So I would like to add at least one sentence to hot_standby_feedback to warn about this too, patch attached.
> >
>
> IIUC, this issue doesn't occur because the primary and standby clocks
> are not synchronized. It happened because the clock on standby moved
> backward.

In PG17 it would be because the clock moved way forward too much on
the standby. I don't know how it happened to that customer, but it was
probably done somehow by the hypervisor in that scenario (so time
wasn't slewed improperly by ntpd AFAIR, edge case, I know...)

> This is quite unlike the 'recovery_min_apply_delay' where
> non-synchronization of clocks between primary and standby can lead to
> unexpected results. This is because we don't compare any time on the
> primary with the time on standby. If this understanding is correct
> then the wording proposed by your patch should be changed accordingly.

.. if my understanding is correct, it is both depending on version :^)
I was thinking about backpatching docs (of what is the recommended
policy here? to just update new-release docs?), so I'm proposing
something more generic than earlier, but it takes Your point into
account - would something like below be good enough?

-       <para>
-        Using this option requires the primary and standby(s) to have system
-        clocks synchronized, otherwise it may lead to prolonged risk of not
-        removing dead rows on primary for extended periods of time as the
-        feedback mechanism is based on timestamps exchanged between primary
-        and standby(s).
-       </para>

+       <para>
+        Using this option requires the primary and standby(s) to have system
+        clocks synchronized (without big time jumps), otherwise it may lead to
+        prolonged risk of not removing dead rows on primary for
extended periods
+        of time as the feedback mechanism implementation is timestamp based.
+       </para>

-J.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-03 05:26  Amit Kapila <[email protected]>
  parent: Jakub Wartak <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Amit Kapila @ 2025-03-03 05:26 UTC (permalink / raw)
  To: Jakub Wartak <[email protected]>; +Cc: pgsql-hackers

On Wed, Jan 8, 2025 at 6:20 PM Jakub Wartak
<[email protected]> wrote:
>
> On Wed, Dec 18, 2024 at 10:33 AM Amit Kapila <[email protected]> wrote:
>
> Hi Amit!
>
> > On Thu, Dec 5, 2024 at 3:14 PM Jakub Wartak
> > <[email protected]> wrote:
> > >
> > > One of our customers ran into a very odd case, where hot standby feedback backend_xmin propagation stopped working due to major (hours/days) clock time shifts on hypervisor-managed VMs. This happens (and is fully reproducible) e.g. in scenarios where standby connects and its own VM is having time from the future (relative to primary) and then that time goes back to "normal". In such situation "sends hot_standby_feedback xmin" timestamp messages are stopped being transferred, e.g.:
> > >
> > > 2024-12-05 02:02:35 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
> > > 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending write 6/E9015230 flush 6/E9015230 apply 6/E9015230
> > > 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 catalog_xmin_epoch 0
> > > <-- clock readjustment and no further "sending hot standby feedback"
> > ...
> > >
> > > I can share reproduction steps if anyone is interested. This basically happens due to usage of TimestampDifferenceExceeds() in XLogWalRcvSendHSFeedback(), but I bet there are other similiar scenarios.
> > >
> >
> > We started to use a different mechanism in HEAD. See XLogWalRcvSendHSFeedback().
>
> Yes, you are correct somewhat because I was looking on REL13_STABLE,
> but I've taken a fresh quick look at 05a7be93558 and tested it too.
> Sadly, PG17 still maintains the same behavior of lack of proper
> backend_xmin propagation (it stops sending hot standby feedback once
> time on standby jumps forward). I believe this is the case because
> walreceiver schedules next wakeup in far far future (when the clock /
> now() is way ahead, see WalRcvComputeNextWakeup()), so when the clock
> is back to normal (resetted back -X hours/days), the next wakeup seems
> to be +X hours/days ahead.
>
> > > What I was kind of surprised about was the lack of recommendation for having primary/standby to have clocks synced when using hot_standby_feedback, but such a thing is mentioned for recovery_min_apply_delay. So I would like to add at least one sentence to hot_standby_feedback to warn about this too, patch attached.
> > >
> >
> > IIUC, this issue doesn't occur because the primary and standby clocks
> > are not synchronized. It happened because the clock on standby moved
> > backward.
>
> In PG17 it would be because the clock moved way forward too much on
> the standby. I don't know how it happened to that customer, but it was
> probably done somehow by the hypervisor in that scenario (so time
> wasn't slewed improperly by ntpd AFAIR, edge case, I know...)
>
> > This is quite unlike the 'recovery_min_apply_delay' where
> > non-synchronization of clocks between primary and standby can lead to
> > unexpected results. This is because we don't compare any time on the
> > primary with the time on standby. If this understanding is correct
> > then the wording proposed by your patch should be changed accordingly.
>
> .. if my understanding is correct, it is both depending on version :^)
>

AFAICS, it doesn't depend on the version. I checked the code of PG13,
and it uses a similar implementation. I am referring to the below code
in PG13:
if (!immed)
{
/*
* Send feedback at most once per wal_receiver_status_interval.
*/
if (!TimestampDifferenceExceeds(sendTime, now,
wal_receiver_status_interval * 1000))
return;
sendTime = now;
}

> I was thinking about backpatching docs (of what is the recommended
> policy here? to just update new-release docs?), so I'm proposing
> something more generic than earlier, but it takes Your point into
> account - would something like below be good enough?
>
> -       <para>
> -        Using this option requires the primary and standby(s) to have system
> -        clocks synchronized, otherwise it may lead to prolonged risk of not
> -        removing dead rows on primary for extended periods of time as the
> -        feedback mechanism is based on timestamps exchanged between primary
> -        and standby(s).
> -       </para>
>
> +       <para>
> +        Using this option requires the primary and standby(s) to have system
> +        clocks synchronized (without big time jumps), otherwise it may lead to
> +        prolonged risk of not removing dead rows on primary for
> extended periods
> +        of time as the feedback mechanism implementation is timestamp based.
> +       </para>
>

How about something like: "Note that if the clock on standby is moved
ahead or backward, the feedback message may not be sent at the
required interval. This can lead to prolonged risk of not removing
dead rows on primary for extended periods as the feedback mechanism is
based on timestamp."

-- 
With Regards,
Amit Kapila.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-03 07:35  Jakub Wartak <[email protected]>
  parent: Amit Kapila <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Jakub Wartak @ 2025-03-03 07:35 UTC (permalink / raw)
  To: Amit Kapila <[email protected]>; +Cc: pgsql-hackers

Hi Amit,

On Mon, Mar 3, 2025 at 6:26 AM Amit Kapila <[email protected]> wrote:
[..]

OK, sure.

> How about something like: "Note that if the clock on standby is moved
> ahead or backward, the feedback message may not be sent at the
> required interval. This can lead to prolonged risk of not removing
> dead rows on primary for extended periods as the feedback mechanism is
> based on timestamp."

Sure thing. I've just added '(..) In the extreme cases this can..' as
it is pretty rare to hit it. Patch attached.

-J.


Attachments:

  [application/octet-stream] v2-0001-doc-Mention-clock-synchronization-recommendation-.patch (1.3K, ../../CAKZiRmyEBkR5tfwrzXaoC6D29Gp6g_fD6Bd_k58DjZV1=rbKdQ@mail.gmail.com/2-v2-0001-doc-Mention-clock-synchronization-recommendation-.patch)
  download | inline diff:
From e9127f866ea4589d8dd4d257f467ecfcaac5936f Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Mon, 3 Mar 2025 08:32:28 +0100
Subject: [PATCH v2] doc: Mention clock synchronization recommendation for
 hot_standby_feedback

hot_standby_feedback mechanics assume that clocks are synchronized, but it was not clear from documentation.

Discussion: https://postgr.es/m/CAKZiRmwBcALLrDgCyEhHP1enUxtPMjyNM_d1A2Lng3_6Rf4Qfw%40mail.gmail.com
---
 doc/src/sgml/config.sgml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 007746a4429..5ebfcdbffc0 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4969,6 +4969,13 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
         until it eventually reaches the primary.  Standbys make no other use
         of feedback they receive other than to pass upstream.
        </para>
+       <para>
+        Note that if the clock on standby is moved ahead or backward, the
+        feedback message may not be sent at the required interval. In extreme
+        cases this can lead to prolonged risk of not removing dead rows on
+        primary for extended periods as the feedback mechanism is based on
+        timestamp.
+       </para>
       </listitem>
      </varlistentry>
 
-- 
2.39.5



^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-03 09:48  Fujii Masao <[email protected]>
  parent: Jakub Wartak <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Fujii Masao @ 2025-03-03 09:48 UTC (permalink / raw)
  To: Jakub Wartak <[email protected]>; Amit Kapila <[email protected]>; +Cc: pgsql-hackers



On 2025/03/03 16:35, Jakub Wartak wrote:
> Hi Amit,
> 
> On Mon, Mar 3, 2025 at 6:26 AM Amit Kapila <[email protected]> wrote:
> [..]
> 
> OK, sure.
> 
>> How about something like: "Note that if the clock on standby is moved
>> ahead or backward, the feedback message may not be sent at the
>> required interval. This can lead to prolonged risk of not removing
>> dead rows on primary for extended periods as the feedback mechanism is
>> based on timestamp."
> 
> Sure thing. I've just added '(..) In the extreme cases this can..' as
> it is pretty rare to hit it. Patch attached.

When the clock moves forward or backward, couldn't it affect
not only the standby but also the primary? I’m wondering
because TimestampDifferenceExceeds() seems to be used
in several places in addition to hot standby feedback.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION







^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-04 03:59  Amit Kapila <[email protected]>
  parent: Fujii Masao <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Amit Kapila @ 2025-03-04 03:59 UTC (permalink / raw)
  To: Fujii Masao <[email protected]>; +Cc: Jakub Wartak <[email protected]>; pgsql-hackers

On Mon, Mar 3, 2025 at 3:18 PM Fujii Masao <[email protected]> wrote:
>
> On 2025/03/03 16:35, Jakub Wartak wrote:
> > Hi Amit,
> >
> > On Mon, Mar 3, 2025 at 6:26 AM Amit Kapila <[email protected]> wrote:
> > [..]
> >
> > OK, sure.
> >
> >> How about something like: "Note that if the clock on standby is moved
> >> ahead or backward, the feedback message may not be sent at the
> >> required interval. This can lead to prolonged risk of not removing
> >> dead rows on primary for extended periods as the feedback mechanism is
> >> based on timestamp."
> >
> > Sure thing. I've just added '(..) In the extreme cases this can..' as
> > it is pretty rare to hit it. Patch attached.
>
> When the clock moves forward or backward, couldn't it affect
> not only the standby but also the primary? I’m wondering
> because TimestampDifferenceExceeds() seems to be used
> in several places in addition to hot standby feedback.
>

Right, it could impact other places as well, like background WAL flush
being delayed. So, what should we do about this? Shall we leave this
as is, make a general statement, find all cases and make a note about
them in docs, do it for the important ones where the impact is more,
or something else?

-- 
With Regards,
Amit Kapila.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-04 11:14  Jakub Wartak <[email protected]>
  parent: Amit Kapila <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Jakub Wartak @ 2025-03-04 11:14 UTC (permalink / raw)
  To: Amit Kapila <[email protected]>; +Cc: Fujii Masao <[email protected]>; pgsql-hackers

Hi,

On Tue, Mar 4, 2025 at 4:59 AM Amit Kapila <[email protected]> wrote:

> > >
> > > Sure thing. I've just added '(..) In the extreme cases this can..' as
> > > it is pretty rare to hit it. Patch attached.
> >
> > When the clock moves forward or backward, couldn't it affect
> > not only the standby but also the primary? I’m wondering
> > because TimestampDifferenceExceeds() seems to be used
> > in several places in addition to hot standby feedback.
> >
>
> Right, it could impact other places as well, like background WAL flush
> being delayed. So, what should we do about this? Shall we leave this
> as is, make a general statement, find all cases and make a note about
> them in docs, do it for the important ones where the impact is more,
> or something else?

Given the occurrence of such conditions is almost close to 0, we could
just open a new separate doc thread/cfentry if somebody is concerned
and add some general statement that OS time should not jump too much
(in some installation section), that it should be slewed (gradually
adjusted) instead. If someone has time jumping on his box back and
forth and something stops working , I still think he has bigger issues
(e.g. now() reflecting wrong data). I would stay vague as much as
possible, because every installation seems to use something different
(hypervisor, kernel modules, ntpd vs ntpd -x and so on).

The problem here was that standby was deteriorating primary (so you
couldn't see easily on primary what could be causing this), so IMHO
patch is fine as it stands, it just adds another not so known reason
to the pool of knowledge why backend_xmin might stop propagating.

-J.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-05 06:15  Amit Kapila <[email protected]>
  parent: Jakub Wartak <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Amit Kapila @ 2025-03-05 06:15 UTC (permalink / raw)
  To: Jakub Wartak <[email protected]>; +Cc: Fujii Masao <[email protected]>; pgsql-hackers

On Tue, Mar 4, 2025 at 4:44 PM Jakub Wartak
<[email protected]> wrote:
>
> On Tue, Mar 4, 2025 at 4:59 AM Amit Kapila <[email protected]> wrote:
>
> > > >
> > > > Sure thing. I've just added '(..) In the extreme cases this can..' as
> > > > it is pretty rare to hit it. Patch attached.
> > >
> > > When the clock moves forward or backward, couldn't it affect
> > > not only the standby but also the primary? I’m wondering
> > > because TimestampDifferenceExceeds() seems to be used
> > > in several places in addition to hot standby feedback.
> > >
> >
> > Right, it could impact other places as well, like background WAL flush
> > being delayed. So, what should we do about this? Shall we leave this
> > as is, make a general statement, find all cases and make a note about
> > them in docs, do it for the important ones where the impact is more,
> > or something else?
>
> Given the occurrence of such conditions is almost close to 0, we could
> just open a new separate doc thread/cfentry if somebody is concerned
> and add some general statement that OS time should not jump too much
> (in some installation section), that it should be slewed (gradually
> adjusted) instead. If someone has time jumping on his box back and
> forth and something stops working , I still think he has bigger issues
> (e.g. now() reflecting wrong data). I would stay vague as much as
> possible, because every installation seems to use something different
> (hypervisor, kernel modules, ntpd vs ntpd -x and so on).
>
> The problem here was that standby was deteriorating primary (so you
> couldn't see easily on primary what could be causing this), so IMHO
> patch is fine as it stands, it just adds another not so known reason
> to the pool of knowledge why backend_xmin might stop propagating.
>

I can go with the last patch as you observed that in a real-world
case, and we can look at others (if any) on a case-to-case basis.
Fujii-San, others, do you have any opinion on this?

-- 
With Regards,
Amit Kapila.






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-14 10:31  vignesh C <[email protected]>
  parent: Amit Kapila <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: vignesh C @ 2025-03-14 10:31 UTC (permalink / raw)
  To: Amit Kapila <[email protected]>; +Cc: Jakub Wartak <[email protected]>; Fujii Masao <[email protected]>; pgsql-hackers

On Wed, 5 Mar 2025 at 11:46, Amit Kapila <[email protected]> wrote:
>
> On Tue, Mar 4, 2025 at 4:44 PM Jakub Wartak
>
> I can go with the last patch as you observed that in a real-world
> case, and we can look at others (if any) on a case-to-case basis.
> Fujii-San, others, do you have any opinion on this?

+1 for this approach.

Regards,
Vignesh





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-24 09:24  Jakub Wartak <[email protected]>
  parent: vignesh C <[email protected]>
  0 siblings, 1 reply; 34+ messages in thread

From: Jakub Wartak @ 2025-03-24 09:24 UTC (permalink / raw)
  To: vignesh C <[email protected]>; +Cc: Amit Kapila <[email protected]>; Fujii Masao <[email protected]>; pgsql-hackers

On Fri, Mar 14, 2025 at 11:31 AM vignesh C <[email protected]> wrote:
>
> On Wed, 5 Mar 2025 at 11:46, Amit Kapila <[email protected]> wrote:
> >
> > On Tue, Mar 4, 2025 at 4:44 PM Jakub Wartak
> >
> > I can go with the last patch as you observed that in a real-world
> > case, and we can look at others (if any) on a case-to-case basis.
> > Fujii-San, others, do you have any opinion on this?
>
> +1 for this approach.
>

OK, so I have set this as Ready for Committer (I assume everybody who
wanted to take voice already did).

-J.





^ permalink  raw  reply  [nested|flat] 34+ messages in thread

* Re: doc: Mention clock synchronization recommendation for hot_standby_feedback
@ 2025-03-31 14:58  Peter Eisentraut <[email protected]>
  parent: Jakub Wartak <[email protected]>
  0 siblings, 0 replies; 34+ messages in thread

From: Peter Eisentraut @ 2025-03-31 14:58 UTC (permalink / raw)
  To: Jakub Wartak <[email protected]>; vignesh C <[email protected]>; +Cc: Amit Kapila <[email protected]>; Fujii Masao <[email protected]>; pgsql-hackers

On 24.03.25 10:24, Jakub Wartak wrote:
> On Fri, Mar 14, 2025 at 11:31 AM vignesh C <[email protected]> wrote:
>>
>> On Wed, 5 Mar 2025 at 11:46, Amit Kapila <[email protected]> wrote:
>>>
>>> On Tue, Mar 4, 2025 at 4:44 PM Jakub Wartak
>>>
>>> I can go with the last patch as you observed that in a real-world
>>> case, and we can look at others (if any) on a case-to-case basis.
>>> Fujii-San, others, do you have any opinion on this?
>>
>> +1 for this approach.
>>
> 
> OK, so I have set this as Ready for Committer (I assume everybody who
> wanted to take voice already did).

committed






^ permalink  raw  reply  [nested|flat] 34+ messages in thread

end of thread, other threads:[~2025-03-31 14:58 UTC | newest]

Thread overview: 34+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 22:30 [PATCH 3/6] Be less verbose on variable names Alvaro Herrera <[email protected]>
2024-02-12 23:13 [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v12 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-02-12 23:13 [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2024-12-05 09:43 doc: Mention clock synchronization recommendation for hot_standby_feedback Jakub Wartak <[email protected]>
2024-12-05 15:06 ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Euler Taveira <[email protected]>
2024-12-05 15:17   ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Andres Freund <[email protected]>
2024-12-09 10:52   ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Jakub Wartak <[email protected]>
2024-12-18 09:33 ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Amit Kapila <[email protected]>
2025-01-08 12:49   ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Jakub Wartak <[email protected]>
2025-03-03 05:26     ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Amit Kapila <[email protected]>
2025-03-03 07:35       ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Jakub Wartak <[email protected]>
2025-03-03 09:48         ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Fujii Masao <[email protected]>
2025-03-04 03:59           ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Amit Kapila <[email protected]>
2025-03-04 11:14             ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Jakub Wartak <[email protected]>
2025-03-05 06:15               ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Amit Kapila <[email protected]>
2025-03-14 10:31                 ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback vignesh C <[email protected]>
2025-03-24 09:24                   ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Jakub Wartak <[email protected]>
2025-03-31 14:58                     ` Re: doc: Mention clock synchronization recommendation for hot_standby_feedback Peter Eisentraut <[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