public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v3 6/7] TupleHashTable.entrysize was unused except for instrumentation..
17+ messages / 6 participants
[nested] [flat]
* [PATCH v9 7/8] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 009d27b9a8..98fd4bf8bd 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -183,7 +183,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 10239aea4f..bca77d52b6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -730,7 +730,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.17.0
--wwtQuX191/I956S7
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0008-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v4 6/7] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index de0205f..d76a630 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -186,7 +186,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 87b1127..3f9ac5e 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -727,7 +727,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.7.4
--jI8keyz6grp/JLjh
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0007-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v7 6/9] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 009d27b9a8..98fd4bf8bd 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -183,7 +183,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 11fe866c1c..3a335d87a0 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -726,7 +726,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.17.0
--UfEAyuTBtIjiZzX6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0007-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v5 6/7] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index de0205f..d76a630 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -186,7 +186,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 11fe866..3a335d8 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -726,7 +726,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.7.4
--M0YLxmUXciMpOLPE
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0007-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v3 6/7] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index de0205f..d76a630 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -186,7 +186,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index df56330..6eac7de 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -724,7 +724,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.7.4
--SNIs70sCzqvszXB4
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v3-0007-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v6 6/7] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 009d27b9a8..98fd4bf8bd 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -183,7 +183,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 11fe866c1c..3a335d87a0 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -726,7 +726,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.17.0
--xHbokkKX1kTiQeDC
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0007-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v2 6/7] TupleHashTable.entrysize was unused except for instrumentation..
@ 2020-02-15 23:19 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Justin Pryzby @ 2020-02-15 23:19 UTC (permalink / raw)
---
src/backend/executor/execGrouping.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index de0205f..d76a630 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -186,7 +186,6 @@ BuildTupleHashTableExt(PlanState *parent,
hashtable->tab_collations = collations;
hashtable->tablecxt = tablecxt;
hashtable->tempcxt = tempcxt;
- hashtable->entrysize = entrysize;
hashtable->tableslot = NULL; /* will be made on first lookup */
hashtable->inputslot = NULL;
hashtable->in_hash_funcs = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f8c93dd..2bcd140 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -724,7 +724,6 @@ typedef struct TupleHashTableData
Oid *tab_collations; /* collations for hash and comparison */
MemoryContext tablecxt; /* memory context containing table */
MemoryContext tempcxt; /* context for function evaluations */
- Size entrysize; /* actual size to make each hash entry */
TupleTableSlot *tableslot; /* slot for referencing table entries */
/* The following fields are set transiently for each table search: */
TupleTableSlot *inputslot; /* current input tuple's slot */
--
2.7.4
--DiL7RhKs8rK9YGuF
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0007-Update-comment-obsolete-since-69c3936a.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
@ 2025-03-26 10:15 Ajin Cherian <[email protected]>
2025-03-26 12:51 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Ajin Cherian @ 2025-03-26 10:15 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers <[email protected]>
On Wed, Mar 26, 2025 at 9:11 PM cca5507 <[email protected]> wrote:
>
> Hi,
>
> Thanks for the comments!
>
> Here are the new version patches, I think it will be more clear.
>
> --
+ {
+ /*
+ * Note that during or after BUILDING_SNAPSHOT, we need handle the xlog
+ * that might mark a transaction as catalog modifying because
the snapshot
+ * only tracks catalog modifying transactions. The transaction before
+ * BUILDING_SNAPSHOT will not be tracked anyway(see
SnapBuildCommitTxn()
+ * for details), so just return.
+ */
+ if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT)
+ {
+ /* Currently only XLOG_HEAP2_NEW_CID means a catalog modifying */
+ if (info == XLOG_HEAP2_NEW_CID && TransactionIdIsValid(xid))
+ ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
buf->origptr);
+ }
Any reason why you avoided calling SnapBuildProcessNewCid here and
only called ReorderBufferXidSetCatalogChanges? If any, please mention
in the comments the reason.
regards,
Ajin Cherian
Fujitsu Australia
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
2025-03-26 10:15 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
@ 2025-03-26 12:51 ` =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: =?ISO-8859-1?B?Y2NhNTUwNw==?= @ 2025-03-26 12:51 UTC (permalink / raw)
To: =?ISO-8859-1?B?QWppbiBDaGVyaWFu?= <[email protected]>; +Cc: =?ISO-8859-1?B?QmVydHJhbmQgRHJvdXZvdA==?= <[email protected]>; =?ISO-8859-1?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; =?ISO-8859-1?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>
Hi,
There are 3 threads currently.
The latest patches are v4-0001 and v3-0002 in [2].
[1]https://www.postgresql.org/message-id/flat/[email protected]
[2]https://www.postgresql.org/message-id/flat/[email protected]
[3]https://www.postgresql.org/message-id/flat/[email protected]
--
Regards,
ChangAo Chen
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
@ 2025-03-27 02:30 Ajin Cherian <[email protected]>
2025-03-27 03:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Ajin Cherian @ 2025-03-27 02:30 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers <[email protected]>
On Thu, Mar 27, 2025 at 1:13 PM cca5507 <[email protected]> wrote:
>
> Hi,
>
> - IIUC your "fast forward" concern is not related to this particular thread but you
> - think it's already an issue on the master branch (outside of the BUILDING_SNAPSHOT
> - handling we are discussing here), is that correct? (that's also what your coding
> - changes makes me think of). If so, I'd suggest to open a dedicated thread for that
> - particular "fast forward" point and do the coding in the current thread as if the
> - fast forward is not an issue.
>
> - Does that make sense?
>
> Yes.
>
> But I think the v4-0001 in [1] is fine.
>
@@ -1301,6 +1335,7 @@ DecodeTXNNeedSkip(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf,
Oid txn_dbid, RepOriginId origin_id)
{
if (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
+ SnapBuildCurrentState(ctx->snapshot_builder) < SNAPBUILD_CONSISTENT ||
(txn_dbid != InvalidOid && txn_dbid != ctx->slot->data.database) ||
FilterByOrigin(ctx, origin_id))
return true;
I don't see an explanation as to why this change was added? Maybe I
missed it in the multiple threads. With this, we are no longer
decoding changes which were in the state SNAPBUILD_FULL_SNAPSHOT. Why
did that change?
regards,
Ajin Cherian
Fujitsu Australia
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
2025-03-27 02:30 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
@ 2025-03-27 03:47 ` =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
2025-03-27 04:57 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: =?ISO-8859-1?B?Y2NhNTUwNw==?= @ 2025-03-27 03:47 UTC (permalink / raw)
To: =?ISO-8859-1?B?QWppbiBDaGVyaWFu?= <[email protected]>; +Cc: =?ISO-8859-1?B?QmVydHJhbmQgRHJvdXZvdA==?= <[email protected]>; =?ISO-8859-1?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; =?ISO-8859-1?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>
Hi,
According to the comment above DecodeTXNNeedSkip(), transactions committed before SNAPBUILD_CONSISTENT state won't be decoded. It's important for initial table data synchronization because the table sync workers use the consistent snapshot to copy data so transactions before SNAPBUILD_CONSISTENT are already included in the initial data.
This change may be redundant with SnapBuildXactNeedsSkip(), I add just for safe.
--
Regards,
ChangAo Chen
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
2025-03-27 02:30 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-03-27 03:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
@ 2025-03-27 04:57 ` Ajin Cherian <[email protected]>
2025-03-27 07:26 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Ajin Cherian @ 2025-03-27 04:57 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers <[email protected]>
On Thu, Mar 27, 2025 at 2:47 PM cca5507 <[email protected]> wrote:
>
> Hi,
>
> According to the comment above DecodeTXNNeedSkip(), transactions committed before SNAPBUILD_CONSISTENT state won't be decoded. It's important for initial table data synchronization because the table sync workers use the consistent snapshot to copy data so transactions before SNAPBUILD_CONSISTENT are already included in the initial data.
>
> This change may be redundant with SnapBuildXactNeedsSkip(), I add just for safe.
>
> --
I understand your explanation but was there a bug on HEAD that this is
solving? If so, can you write a test case to show this? This will make
it more convincing that this change as well is required. Your other
change has a test case to confirm, but this doesn't.
regards,
Ajin Cherian
Fujitsu Australia
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
2025-03-27 02:30 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-03-27 03:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
2025-03-27 04:57 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
@ 2025-03-27 07:26 ` =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: =?ISO-8859-1?B?Y2NhNTUwNw==?= @ 2025-03-27 07:26 UTC (permalink / raw)
To: =?ISO-8859-1?B?QWppbiBDaGVyaWFu?= <[email protected]>; +Cc: =?ISO-8859-1?B?QmVydHJhbmQgRHJvdXZvdA==?= <[email protected]>; =?ISO-8859-1?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; =?ISO-8859-1?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>
Hi,
This change doesn't solve a bug. But I think it makes the code and comments more consistent. I currently have no idea about how to test it.
--
Regards,
ChangAo Chen
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
@ 2025-06-27 10:29 Ajin Cherian <[email protected]>
2025-06-28 03:44 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?gb18030?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Ajin Cherian @ 2025-06-27 10:29 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers <[email protected]>
On Fri, Jun 27, 2025 at 3:48 PM cca5507 <[email protected]> wrote:
>
> Hi,
>
> I refactor the code and fix the git apply warning according to [1].
>
> Here are the new version patches.
>
> --
> Regards,
> ChangAo Chen
>
> [1] https://www.postgresql.org/message-id/Zrmh7X8jYCbFYXjH%40ip-10-97-1-34.eu-west-3.compute.internal
I see this problem is similar to the bug reported in [1], and your fix
also addresses the issue reported there. Although I like your approach
of tracking changes starting from the BUILDING_SNAPSHOT state, I’d
like to suggest an alternative.
While debugging that issue, my plan was not to track catalog changes
prior to SNAPBUILD_CONSISTENT, but instead to ensure we don’t use
snapshots built before SNAPBUILD_CONSISTENT, since we don’t track
catalog changes in those states. We should discard previously built
snapshots and rebuild them once we reach the SNAPBUILD_CONSISTENT
state. At that point, all necessary transactions would have been
committed, and builder->xmin would have advanced enough to decode all
transactions from then on.
The problem is that previously built snapshots hang around without the
latest xmin and xmax, and we tend to reuse them. We should ensure that
all txn->base_snapshot and builder->snapshot snapshots built in the
SNAPBUILD_FULL_SNAPSHOT state are rebuilt once we reach
SNAPBUILD_CONSISTENT. For this, we need to track when the snapshot was
built. There is already a field in ReorderBufferTXN -
'base_snapshot_lsn' which we can use. If base_snapshot_lsn <
builder->start_decoding_at, then we should rebuild the snapshot. Just
a thought.
regards,
Ajin Cherian
Fujitsu Australia
[1] - https://www.postgresql.org/message-id/18509-983f064d174ea880%40postgresql.org
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
2025-06-27 10:29 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
@ 2025-06-28 03:44 ` =?gb18030?B?Y2NhNTUwNw==?= <[email protected]>
2025-07-01 23:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Michael Paquier <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: =?gb18030?B?Y2NhNTUwNw==?= @ 2025-06-28 03:44 UTC (permalink / raw)
To: =?gb18030?B?QWppbiBDaGVyaWFu?= <[email protected]>; +Cc: =?gb18030?B?QmVydHJhbmQgRHJvdXZvdA==?= <[email protected]>; =?gb18030?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; =?gb18030?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>
Hi,
If I understand correctly, this may break the basic principle:
The aim is to build a snapshot that behaves the same as a freshly taken MVCC snapshot would have at the time the XLogRecord was generated.
--
Regards,
ChangAo Chen
------------------ Original ------------------
From: "Ajin Cherian" <[email protected]>;
Date: Fri, Jun 27, 2025 06:29 PM
To: "cca5507"<[email protected]>;
Cc: "Bertrand Drouvot"<[email protected]>;"Michael Paquier"<[email protected]>;"pgsql-hackers"<[email protected]>;
Subject: Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
On Fri, Jun 27, 2025 at 3:48 PM cca5507 <[email protected]> wrote:
>
> Hi,
>
> I refactor the code and fix the git apply warning according to [1].
>
> Here are the new version patches.
>
> --
> Regards,
> ChangAo Chen
>
> [1] https://www.postgresql.org/message-id/Zrmh7X8jYCbFYXjH%40ip-10-97-1-34.eu-west-3.compute.internal
I see this problem is similar to the bug reported in [1], and your fix
also addresses the issue reported there. Although I like your approach
of tracking changes starting from the BUILDING_SNAPSHOT state, I’d
like to suggest an alternative.
While debugging that issue, my plan was not to track catalog changes
prior to SNAPBUILD_CONSISTENT, but instead to ensure we don’t use
snapshots built before SNAPBUILD_CONSISTENT, since we don’t track
catalog changes in those states. We should discard previously built
snapshots and rebuild them once we reach the SNAPBUILD_CONSISTENT
state. At that point, all necessary transactions would have been
committed, and builder->xmin would have advanced enough to decode all
transactions from then on.
The problem is that previously built snapshots hang around without the
latest xmin and xmax, and we tend to reuse them. We should ensure that
all txn->base_snapshot and builder->snapshot snapshots built in the
SNAPBUILD_FULL_SNAPSHOT state are rebuilt once we reach
SNAPBUILD_CONSISTENT. For this, we need to track when the snapshot was
built. There is already a field in ReorderBufferTXN -
'base_snapshot_lsn' which we can use. If base_snapshot_lsn <
builder->start_decoding_at, then we should rebuild the snapshot. Just
a thought.
regards,
Ajin Cherian
Fujitsu Australia
[1] - https://www.postgresql.org/message-id/18509-983f064d174ea880%40postgresql.org
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
2025-06-27 10:29 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-06-28 03:44 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?gb18030?B?Y2NhNTUwNw==?= <[email protected]>
@ 2025-07-01 23:47 ` Michael Paquier <[email protected]>
2025-11-21 08:16 ` Re: Historic snapshot doesn't track txns committed inBUILDING_SNAPSHOT state =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2025-07-01 23:47 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Ajin Cherian <[email protected]>; Bertrand Drouvot <[email protected]>; pgsql-hackers <[email protected]>
On Sat, Jun 28, 2025 at 11:44:52AM +0800, cca5507 wrote:
> If I understand correctly, this may break the basic principle:
>
> The aim is to build a snapshot that behaves the same as a
> freshly taken MVCC snapshot would have at the time the
> XLogRecord was generated.
Please note that we prefer bottom-posting when sending messages on
pgsql-hackers and the community mailing lists, as defined in this
link:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting
Top-posting, as you did in your last email, is breaking the logic and
flow of the discussion.
Some more useful documentation from the PostgreSQL wiki:
https://wiki.postgresql.org/wiki/Mailing_Lists
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Historic snapshot doesn't track txns committed inBUILDING_SNAPSHOT state
2025-06-27 10:29 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-06-28 03:44 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?gb18030?B?Y2NhNTUwNw==?= <[email protected]>
2025-07-01 23:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Michael Paquier <[email protected]>
@ 2025-11-21 08:16 ` =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: =?utf-8?B?Y2NhNTUwNw==?= @ 2025-11-21 08:16 UTC (permalink / raw)
To: =?utf-8?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; +Cc: =?utf-8?B?QWppbiBDaGVyaWFu?= <[email protected]>; =?utf-8?B?QmVydHJhbmQgRHJvdXZvdA==?= <[email protected]>; =?utf-8?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>
Hi all,
Attach the rebased patches.
Some useful link:
Discussion:
https://www.postgresql.org/message-id/flat/[email protected]
https://www.postgresql.org/message-id/flat/[email protected]
https://www.postgresql.org/message-id/flat/[email protected]
Bug Report:
https://www.postgresql.org/message-id/flat/18509-983f064d174ea880%40postgresql.org
https://www.postgresql.org/message-id/flat/19109-4a9ce36715d69632%40postgresql.org
CF Entry:
https://commitfest.postgresql.org/patch/5029/
--
Regards,
ChangAo Chen
Attachments:
[application/octet-stream] v6-0001-Track-transactions-committed-in-BUILDING_SNAPSHOT.patch (3.9K, ../../[email protected]/3-v6-0001-Track-transactions-committed-in-BUILDING_SNAPSHOT.patch)
download | inline diff:
From 12dd3434ef13609b324bbbbe68a3f0e2a48934a2 Mon Sep 17 00:00:00 2001
From: ChangAo Chen <[email protected]>
Date: Fri, 21 Nov 2025 15:19:22 +0800
Subject: [PATCH v6 1/2] Track transactions committed in BUILDING_SNAPSHOT.
The historic snapshot previously didn't track transactions committed
in BUILDING_SNAPSHOT, this might result in a transaction taking an
incorrect snapshot and logical decoding being interrupted. So we need
to track these transactions.
We also need to handle the xlog which means a catalog change in BUILDING_SNAPSHOT
because the historic snapshot only tracks catalog modifying transactions.
---
src/backend/replication/logical/decode.c | 33 ++++++++++++++++++++----
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index cc03f0706e9..de1bed30781 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -206,12 +206,16 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
uint8 info = XLogRecGetInfo(r) & XLOG_XACT_OPMASK;
/*
- * If the snapshot isn't yet fully built, we cannot decode anything, so
- * bail out.
+ * If the snapshot hasn't started building yet, the transaction won't be
+ * decoded or tracked by the snapshot, so bail out.
*/
- if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+ if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT)
return;
+ /*
+ * Note that if the snapshot isn't yet fully built, the xlog is only used
+ * to build the snapshot and won't be decoded.
+ */
switch (info)
{
case XLOG_XACT_COMMIT:
@@ -282,18 +286,24 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
{
TransactionId xid;
xl_xact_invals *invals;
+ bool has_snapshot;
xid = XLogRecGetXid(r);
invals = (xl_xact_invals *) XLogRecGetData(r);
+ has_snapshot =
+ SnapBuildCurrentState(builder) >= SNAPBUILD_FULL_SNAPSHOT;
/*
* Execute the invalidations for xid-less transactions,
* otherwise, accumulate them so that they can be processed at
* the commit time.
+ *
+ * Note that we only need to do this when we are not fast-forwarding
+ * and there is a snapshot.
*/
if (TransactionIdIsValid(xid))
{
- if (!ctx->fast_forward)
+ if (!ctx->fast_forward && has_snapshot)
ReorderBufferAddInvalidations(reorder, xid,
buf->origptr,
invals->nmsgs,
@@ -301,7 +311,7 @@ xact_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
buf->origptr);
}
- else if (!ctx->fast_forward)
+ else if (!ctx->fast_forward && has_snapshot)
ReorderBufferImmediateInvalidation(ctx->reorder,
invals->nmsgs,
invals->msgs);
@@ -419,7 +429,19 @@ heap2_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
* SnapBuildProcessRunningXacts().
*/
if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+ {
+ /*
+ * If we are building snapshot and the xlog means a catalog
+ * change, we need to mark it in the reorder buffer.
+ *
+ * Now only XLOG_HEAP2_NEW_CID means a catalog change.
+ */
+ if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT &&
+ TransactionIdIsValid(xid) && info == XLOG_HEAP2_NEW_CID)
+ ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+
return;
+ }
switch (info)
{
@@ -1306,6 +1328,7 @@ DecodeTXNNeedSkip(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
Oid txn_dbid, RepOriginId origin_id)
{
if (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
+ SnapBuildCurrentState(ctx->snapshot_builder) < SNAPBUILD_CONSISTENT ||
(txn_dbid != InvalidOid && txn_dbid != ctx->slot->data.database) ||
FilterByOrigin(ctx, origin_id))
return true;
--
2.34.1
[application/octet-stream] v6-0002-Add-test-case-snapshot_build-for-test_decoding.patch (5.2K, ../../[email protected]/4-v6-0002-Add-test-case-snapshot_build-for-test_decoding.patch)
download | inline diff:
From d4747efa7c8103ec259a725051fff3bc4849dc17 Mon Sep 17 00:00:00 2001
From: ChangAo Chen <[email protected]>
Date: Fri, 21 Nov 2025 15:48:27 +0800
Subject: [PATCH v6 2/2] Add test case snapshot_build for test_decoding.
---
contrib/test_decoding/Makefile | 3 +-
.../test_decoding/expected/snapshot_build.out | 33 +++++++++++++
contrib/test_decoding/meson.build | 1 +
.../test_decoding/specs/snapshot_build.spec | 46 +++++++++++++++++++
4 files changed, 82 insertions(+), 1 deletion(-)
create mode 100644 contrib/test_decoding/expected/snapshot_build.out
create mode 100644 contrib/test_decoding/specs/snapshot_build.spec
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index acbcaed2feb..60210726566 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -9,7 +9,8 @@ REGRESS = ddl xact rewrite toast permissions decoding_in_xact \
ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
twophase_snapshot slot_creation_error catalog_change_snapshot \
- skip_snapshot_restore invalidation_distribution parallel_session_origin
+ skip_snapshot_restore invalidation_distribution parallel_session_origin \
+ snapshot_build
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
diff --git a/contrib/test_decoding/expected/snapshot_build.out b/contrib/test_decoding/expected/snapshot_build.out
new file mode 100644
index 00000000000..0fcf20cce86
--- /dev/null
+++ b/contrib/test_decoding/expected/snapshot_build.out
@@ -0,0 +1,33 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s1_begin s1_insert s2_init s3_begin s3_insert s4_create s1_commit s4_begin s4_insert s3_commit s4_commit s2_get_changes
+step s1_begin: BEGIN;
+step s1_insert: INSERT INTO tbl1 VALUES (1);
+step s2_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding'); <waiting ...>
+step s3_begin: BEGIN;
+step s3_insert: INSERT INTO tbl1 VALUES (1);
+step s4_create: CREATE TABLE tbl2 (val1 integer);
+step s1_commit: COMMIT;
+step s4_begin: BEGIN;
+step s4_insert: INSERT INTO tbl2 VALUES (1);
+step s3_commit: COMMIT;
+step s2_init: <... completed>
+?column?
+--------
+init
+(1 row)
+
+step s4_commit: COMMIT;
+step s2_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
+data
+------------------------------------------
+BEGIN
+table public.tbl2: INSERT: val1[integer]:1
+COMMIT
+(3 rows)
+
+?column?
+--------
+stop
+(1 row)
+
diff --git a/contrib/test_decoding/meson.build b/contrib/test_decoding/meson.build
index 99310555e6c..252a39b7727 100644
--- a/contrib/test_decoding/meson.build
+++ b/contrib/test_decoding/meson.build
@@ -65,6 +65,7 @@ tests += {
'skip_snapshot_restore',
'invalidation_distribution',
'parallel_session_origin',
+ 'snapshot_build',
],
'regress_args': [
'--temp-config', files('logical.conf'),
diff --git a/contrib/test_decoding/specs/snapshot_build.spec b/contrib/test_decoding/specs/snapshot_build.spec
new file mode 100644
index 00000000000..334531dd219
--- /dev/null
+++ b/contrib/test_decoding/specs/snapshot_build.spec
@@ -0,0 +1,46 @@
+# Test snapshot build correctly, it must track committed transactions during BUILDING_SNAPSHOT
+
+setup
+{
+ DROP TABLE IF EXISTS tbl1;
+ DROP TABLE IF EXISTS tbl2;
+ CREATE TABLE tbl1 (val1 integer);
+}
+
+teardown
+{
+ DROP TABLE tbl1;
+ DROP TABLE tbl2;
+ SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
+}
+
+session "s1"
+setup { SET synchronous_commit=on; }
+step "s1_begin" { BEGIN; }
+step "s1_insert" { INSERT INTO tbl1 VALUES (1); }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+setup { SET synchronous_commit=on; }
+step "s2_init" { SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding'); }
+step "s2_get_changes" { SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0'); }
+
+session "s3"
+setup { SET synchronous_commit=on; }
+step "s3_begin" { BEGIN; }
+step "s3_insert" { INSERT INTO tbl1 VALUES (1); }
+step "s3_commit" { COMMIT; }
+
+session "s4"
+setup { SET synchronous_commit=on; }
+step "s4_create" { CREATE TABLE tbl2 (val1 integer); }
+step "s4_begin" { BEGIN; }
+step "s4_insert" { INSERT INTO tbl2 VALUES (1); }
+step "s4_commit" { COMMIT; }
+
+# T1: s1_begin -> s1_insert -> BUILDING_SNAPSHOT -> s1_commit -> FULL_SNAPSHOT
+# T2: BUILDING_SNAPSHOT -> s3_begin -> s3_insert -> FULL_SNAPSHOT -> s3_commit -> CONSISTENT
+# T3: BUILDING_SNAPSHOT -> s4_create -> FULL_SNAPSHOT
+# T4: FULL_SNAPSHOT -> s4_begin -> s4_insert -> CONSISTENT -> s4_commit
+# The snapshot must track T3 or the replay of T4 will fail because its snapshot cannot see tbl2
+permutation "s1_begin" "s1_insert" "s2_init" "s3_begin" "s3_insert" "s4_create" "s1_commit" "s4_begin" "s4_insert" "s3_commit" "s4_commit" "s2_get_changes"
--
2.34.1
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2025-11-21 08:16 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-15 23:19 [PATCH v9 7/8] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2020-02-15 23:19 [PATCH v4 6/7] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2020-02-15 23:19 [PATCH v7 6/9] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2020-02-15 23:19 [PATCH v5 6/7] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2020-02-15 23:19 [PATCH v3 6/7] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2020-02-15 23:19 [PATCH v6 6/7] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2020-02-15 23:19 [PATCH v2 6/7] TupleHashTable.entrysize was unused except for instrumentation.. Justin Pryzby <[email protected]>
2025-03-26 10:15 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-03-26 12:51 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
2025-03-27 02:30 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-03-27 03:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
2025-03-27 04:57 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-03-27 07:26 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?ISO-8859-1?B?Y2NhNTUwNw==?= <[email protected]>
2025-06-27 10:29 Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Ajin Cherian <[email protected]>
2025-06-28 03:44 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state =?gb18030?B?Y2NhNTUwNw==?= <[email protected]>
2025-07-01 23:47 ` Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state Michael Paquier <[email protected]>
2025-11-21 08:16 ` Re: Historic snapshot doesn't track txns committed inBUILDING_SNAPSHOT state =?utf-8?B?Y2NhNTUwNw==?= <[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