public inbox for [email protected]
help / color / mirror / Atom feedFrom: Drouvot, Bertrand <[email protected]>
To: Robert Haas <[email protected]>
To: Andres Freund <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Ibrar Ahmed <[email protected]>
Cc: Amit Khandekar <[email protected]>
Cc: [email protected]
Cc: tushar <[email protected]>
Cc: Rahila Syed <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Minimal logical decoding on standbys
Date: Wed, 21 Dec 2022 10:06:42 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CA+TgmoY0df9X+5ENg8P0BGj0odhM45sdQ7kB4JMo4NKaoFy-Vg@mail.gmail.com>
<[email protected]>
<CA+Tgmoa1cZSo1tUTzxS+RyniMuNtkpxUeEhnD2+-mvJodVtdSQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<CA+TgmoaVcu_mbxbH=EccvKG6u8+MdQf9zx98uAL9zsStFwrYsQ@mail.gmail.com>
<[email protected]>
<CA+TgmoZ8-DXtDMMDGggFkSYyO8nmNdBKWjV6Ts-zYGR9p5sK5g@mail.gmail.com>
<CA+TgmobmTvEUDVOBzARa9eD9XLqsvDW+v29dV+WvDKZPJAwNTQ@mail.gmail.com>
<CA+TgmoaOsoQuXEo6gMq0GP8SyvCaS=ptYqdYc0uzDMoeShf_nw@mail.gmail.com>
<CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
Hi,
On 12/20/22 10:41 PM, Robert Haas wrote:
> On Tue, Dec 20, 2022 at 3:39 PM Robert Haas <[email protected]> wrote:
>> I think this might be the only WAL record type where there's a
>> problem, but I haven't fully confirmed that yet.
>
> It's not. GIST has the same issue. The same test case demonstrates the
> problem there, if you substitute this test script for
> kpt_hash_setup.sql and possibly also run it for somewhat longer. One
> might think that this wouldn't be a problem, because the comments for
> gistxlogDelete say this:
>
> /*
> * In payload of blk 0 : todelete OffsetNumbers
> */
>
> But it's not in the payload of blk 0. It follows the main payload.
Oh right, nice catch!
Indeed, we can see in gistRedoDeleteRecord():
"
todelete = (OffsetNumber *) ((char *) xldata + SizeOfGistxlogDelete);
"
>
> This is the reverse of xl_heap_freeze_page, which claims that freeze
> plans and offset numbers follow, but they don't: they're in the data
> for block 0.
oh right, we can see in heap_xlog_freeze_page():
"
plans = (xl_heap_freeze_plan *) XLogRecGetBlockData(record, 0, NULL);
offsets = (OffsetNumber *) ((char *) plans +
(xlrec->nplans *
sizeof(xl_heap_freeze_plan)));
"
> xl_btree_delete is also wrong, claiming that the data
> follows when it's really attached to block 0.
oh right, we can see in btree_xlog_delete():
"
char *ptr = XLogRecGetBlockData(record, 0, NULL);
page = (Page) BufferGetPage(buffer);
if (xlrec->nupdated > 0)
{
OffsetNumber *updatedoffsets;
xl_btree_update *updates;
updatedoffsets = (OffsetNumber *)
(ptr + xlrec->ndeleted * sizeof(OffsetNumber));
updates = (xl_btree_update *) ((char *) updatedoffsets +
xlrec->nupdated *
sizeof(OffsetNumber));
"
> I guess whatever else we
> do here, we should fix the comments.
>
Agree, please find attached a patch proposal doing so.
> Bottom line is that I think the two cases that have alignment issues
> as coded are xl_hash_vacuum_one_page and gistxlogDelete. Everything
> else is OK, as far as I can tell right now.
>
Thanks a lot for the repro(s) and explanations! That's very useful/helpful.
Based on your discovery about the wrong comments above, I'm now tempted to fix those 2 alignment issues
by using a FLEXIBLE_ARRAY_MEMBER within those structs (as you proposed in [1]) (as that should also prevent
any possible wrong comments about where the array is located).
What do you think?
[1]: https://www.postgresql.org/message-id/CA%2BTgmoaVcu_mbxbH%3DEccvKG6u8%2BMdQf9zx98uAL9zsStFwrYsQ%40ma...
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 33f1c7e31b..95068feb87 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -52,9 +52,7 @@ typedef struct gistxlogDelete
TransactionId snapshotConflictHorizon;
uint16 ntodelete; /* number of deleted offsets */
- /*
- * In payload of blk 0 : todelete OffsetNumbers
- */
+ /* TODELETE OFFSET NUMBER ARRAY FOLLOWS */
} gistxlogDelete;
#define SizeOfGistxlogDelete (offsetof(gistxlogDelete, ntodelete) + sizeof(uint16))
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index 5c77290eec..1117e95ede 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -345,8 +345,9 @@ typedef struct xl_heap_freeze_page
TransactionId snapshotConflictHorizon;
uint16 nplans;
- /* FREEZE PLANS FOLLOW */
- /* OFFSET NUMBER ARRAY FOLLOWS */
+ /*
+ * In payload of blk 0 : FREEZE PLANS and OFFSET NUMBER ARRAY
+ */
} xl_heap_freeze_page;
#define SizeOfHeapFreezePage (offsetof(xl_heap_freeze_page, nplans) + sizeof(uint16))
diff --git a/src/include/access/nbtxlog.h b/src/include/access/nbtxlog.h
index 3b2d959c69..1aa8e7eca5 100644
--- a/src/include/access/nbtxlog.h
+++ b/src/include/access/nbtxlog.h
@@ -236,9 +236,12 @@ typedef struct xl_btree_delete
uint16 ndeleted;
uint16 nupdated;
- /* DELETED TARGET OFFSET NUMBERS FOLLOW */
- /* UPDATED TARGET OFFSET NUMBERS FOLLOW */
- /* UPDATED TUPLES METADATA (xl_btree_update) ARRAY FOLLOWS */
+ /*
+ * In payload of blk 0 :
+ * - DELETED TARGET OFFSET NUMBERS
+ * - UPDATED TARGET OFFSET NUMBERS
+ * - UPDATED TUPLES METADATA (xl_btree_update) ARRAY
+ */
} xl_btree_delete;
#define SizeOfBtreeDelete (offsetof(xl_btree_delete, nupdated) + sizeof(uint16))
Attachments:
[text/plain] v1-0001-fix-some-comments.patch (1.8K, ../[email protected]/2-v1-0001-fix-some-comments.patch)
download | inline diff:
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 33f1c7e31b..95068feb87 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -52,9 +52,7 @@ typedef struct gistxlogDelete
TransactionId snapshotConflictHorizon;
uint16 ntodelete; /* number of deleted offsets */
- /*
- * In payload of blk 0 : todelete OffsetNumbers
- */
+ /* TODELETE OFFSET NUMBER ARRAY FOLLOWS */
} gistxlogDelete;
#define SizeOfGistxlogDelete (offsetof(gistxlogDelete, ntodelete) + sizeof(uint16))
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index 5c77290eec..1117e95ede 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -345,8 +345,9 @@ typedef struct xl_heap_freeze_page
TransactionId snapshotConflictHorizon;
uint16 nplans;
- /* FREEZE PLANS FOLLOW */
- /* OFFSET NUMBER ARRAY FOLLOWS */
+ /*
+ * In payload of blk 0 : FREEZE PLANS and OFFSET NUMBER ARRAY
+ */
} xl_heap_freeze_page;
#define SizeOfHeapFreezePage (offsetof(xl_heap_freeze_page, nplans) + sizeof(uint16))
diff --git a/src/include/access/nbtxlog.h b/src/include/access/nbtxlog.h
index 3b2d959c69..1aa8e7eca5 100644
--- a/src/include/access/nbtxlog.h
+++ b/src/include/access/nbtxlog.h
@@ -236,9 +236,12 @@ typedef struct xl_btree_delete
uint16 ndeleted;
uint16 nupdated;
- /* DELETED TARGET OFFSET NUMBERS FOLLOW */
- /* UPDATED TARGET OFFSET NUMBERS FOLLOW */
- /* UPDATED TUPLES METADATA (xl_btree_update) ARRAY FOLLOWS */
+ /*
+ * In payload of blk 0 :
+ * - DELETED TARGET OFFSET NUMBERS
+ * - UPDATED TARGET OFFSET NUMBERS
+ * - UPDATED TUPLES METADATA (xl_btree_update) ARRAY
+ */
} xl_btree_delete;
#define SizeOfBtreeDelete (offsetof(xl_btree_delete, nupdated) + sizeof(uint16))
view thread (195+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Minimal logical decoding on standbys
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox