Thread: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
[bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi hackers,
During logical decoding, we have recently observed (and reported in [1]) errors like:
ERROR: could not open relation with OID 0
After investigating a recent issue on a PostgreSQL database that was encountering this error, we found that the logical decoding of relation rewrite with toast could produce this error without resetting the toast_hash.
We were able to create this repro of the error:
postgres=# \! cat bdt_repro.sql
select pg_create_logical_replication_slot('bdt_slot','test_decoding');
CREATE TABLE tbl1 (a INT, b TEXT);
CREATE TABLE tbl2 (a INT);
ALTER TABLE tbl1 ALTER COLUMN b SET STORAGE EXTERNAL;
BEGIN;
INSERT INTO tbl1 VALUES(1, repeat('a', 4000)) ;
ALTER TABLE tbl1 ADD COLUMN id serial primary key;
INSERT INTO tbl2 VALUES(1);
commit;
select * from pg_logical_slot_get_changes('bdt_slot', null, null);
That ends up on 12.5 with:
ERROR: could not open relation with OID 0
And on current master with:server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The issue has been introduced by 325f2ec555 (and more precisely by its change in reorderbuffer.c), so it does affect pre v11 versions:
git branch -r --contains 325f2ec555
origin/HEAD -> origin/master
origin/REL_11_STABLE
origin/REL_12_STABLE
origin/REL_13_STABLE
origin/REL_14_STABLE
origin/master
The fact that current master is producing a different behavior than 12.5 (for example) is due to 4daa140a2f that generates a failed assertion in such a case (after going to the code path that should print out the ERROR):#2 0x0000000000b29fab in ExceptionalCondition (conditionName=0xce6850 "(rb->size >= sz) && (txn->size >= sz)", errorType=0xce5f84 "FailedAssertion", fileName=0xce5fd0 "reorderbuffer.c", lineNumber=3141) at assert.c:69
#3 0x00000000008ff1fb in ReorderBufferChangeMemoryUpdate (rb=0x11a7a40, change=0x11c94b8, addition=false) at reorderbuffer.c:3141
#4 0x00000000008fab27 in ReorderBufferReturnChange (rb=0x11a7a40, change=0x11c94b8, upd_mem=true) at reorderbuffer.c:477
#5 0x0000000000902ec1 in ReorderBufferToastReset (rb=0x11a7a40, txn=0x11b1998) at reorderbuffer.c:4799
#6 0x00000000008faaa2 in ReorderBufferReturnTXN (rb=0x11a7a40, txn=0x11b1998) at reorderbuffer.c:448
#7 0x00000000008fc95b in ReorderBufferCleanupTXN (rb=0x11a7a40, txn=0x11b1998) at reorderbuffer.c:1540
I am adding Amit to this thread to make him aware than this is related to the recent issue Jeremy and I were talking about in [1] - which we now believe is not linked to the logical decoding and speculative insert bug fixed in 4daa140a2f but likely to this new toast rewrite bug.
Please find enclosed a patch proposal to: * Avoid the failed assertion on current master and generate the error message instead (should the code reach that stage).
* Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro is working).
I am adding this patch to the next commitfest.
Thanks
Bertrand
[1]: https://www.postgresql.org/message-id/CAA4eK1KcUPwwhDVhJmdQExc09AzEBZMGbOa-u3DYaJs1zzfEnA%40mail.gmail.com
Attachment
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi Drouvot,
I can reproduce the issue you mentioned on REL_12_STABLE as well as Master branch, but the patch doesn't apply to
REL_12_STABLE.After applied it to Master branch, it returns some wired result when run the query in the first time.
As you can see in the log below, after the first time execute the query `select * from
pg_logical_slot_get_changes('bdt_slot',null, null);` it returns some extra data.
david:postgres$ psql -d postgres
psql (15devel)
Type "help" for help.
postgres=# \q
david:postgres$ psql -d postgres
psql (15devel)
Type "help" for help.
postgres=# select pg_create_logical_replication_slot('bdt_slot','test_decoding');
pg_create_logical_replication_slot
------------------------------------
(bdt_slot,0/1484FA8)
(1 row)
postgres=# CREATE TABLE tbl1 (a INT, b TEXT);
CREATE TABLE
postgres=# CREATE TABLE tbl2 (a INT);
CREATE TABLE
postgres=# ALTER TABLE tbl1 ALTER COLUMN b SET STORAGE EXTERNAL;
ALTER TABLE
postgres=#
postgres=# BEGIN;
BEGIN
postgres=*# INSERT INTO tbl1 VALUES(1, repeat('a', 4000)) ;
INSERT 0 1
postgres=*# ALTER TABLE tbl1 ADD COLUMN id serial primary key;
ALTER TABLE
postgres=*# INSERT INTO tbl2 VALUES(1);
INSERT 0 1
postgres=*# commit;
COMMIT
postgres=#
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null);
lsn | xid |
data
-----------+-----+--------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null);
lsn | xid | data
-----+-----+------
(0 rows)
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null);
lsn | xid | data
-----+-----+------
(0 rows)
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null);
lsn | xid | data
-----+-----+------
(0 rows)
postgres=#
Thank you,
David
Re: [UNVERIFIED SENDER] Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi David,
Hi Drouvot, I can reproduce the issue you mentioned on REL_12_STABLE as well as Master branch,
Thanks for looking at it!
but the patch doesn't apply to REL_12_STABLE.
Indeed this patch version provided is done for the current Master branch.
After applied it to Master branch, it returns some wired result when run the query in the first time.
As you can see in the log below, after the first time execute the query `select * from pg_logical_slot_get_changes('bdt_slot', null, null);` it returns some extra data.
david:postgres$ psql -d postgres
psql (15devel)
Type "help" for help.
postgres=# \q
david:postgres$ psql -d postgres
psql (15devel)
Type "help" for help.
postgres=# select pg_create_logical_replication_slot('bdt_slot','test_decoding'); pg_create_logical_replication_slot
------------------------------------ (bdt_slot,0/1484FA8)
(1 row)
postgres=# CREATE TABLE tbl1 (a INT, b TEXT);
CREATE TABLE
postgres=# CREATE TABLE tbl2 (a INT);
CREATE TABLE
postgres=# ALTER TABLE tbl1 ALTER COLUMN b SET STORAGE EXTERNAL;
ALTER TABLE
postgres=#
postgres=# BEGIN;
BEGIN
postgres=*# INSERT INTO tbl1 VALUES(1, repeat('a', 4000)) ;
INSERT 0 1
postgres=*# ALTER TABLE tbl1 ADD COLUMN id serial primary key;
ALTER TABLE
postgres=*# INSERT INTO tbl2 VALUES(1);
INSERT 0 1
postgres=*# commit;
COMMIT
postgres=#
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null); lsn | xid |
data
-----------+-----+--------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null); lsn | xid | data
-----+-----+------
(0 rows)
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null); lsn | xid | data
-----+-----+------
(0 rows)
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null); lsn | xid | data
-----+-----+------
(0 rows)
postgres=#I don't see extra data in your output and it looks like your copy/paste is missing some content, no?
On my side, that looks good and here is what i get with the patch applied:
psql (15devel)
Type "help" for help.
postgres=# \i repro.sql
pg_create_logical_replication_slot
------------------------------------
(bdt_slot,0/172DAF0)
(1 row)
CREATE TABLE
CREATE TABLE
ALTER TABLE
BEGIN
INSERT 0 1
ALTER TABLE
INSERT 0 1
COMMIT
lsn | xid |
data
-----------+-----+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------
0/172DB40 | 708 | BEGIN 708
0/1753298 | 708 | COMMIT 708
0/17532C8 | 709 | BEGIN 709
0/1754828 | 709 | COMMIT 709
0/1754828 | 710 | BEGIN 710
0/1754B10 | 710 | COMMIT 710
0/1754B10 | 711 | BEGIN 711
0/1755CA0 | 711 | table public.tbl1: INSERT: a[integer]:1 b[text]:'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
0/176F970 | 711 | table public.tbl2: INSERT: a[integer]:1
0/1770688 | 711 | COMMIT 711
(10 rows)
Thanks
Bertrand
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > Please find enclosed a patch proposal to: > > * Avoid the failed assertion on current master and generate the error message instead (should the code reach that stage). > * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro is working). > I think instead of resetting toast_hash for this case why don't we set 'relrewrite' for toast tables as well during rewrite? If we do that then we will simply skip assembling toast chunks for the toast table. In make_new_heap(), we are calling NewHeapCreateToastTable() to create toast table where we can pass additional information (probably 'toastid'), if required to set 'relrewrite'. Additionally, let's add a test case if possible for this. BTW, I see this as an Open Item for PG-14 [1] which seems wrong to me as this is a bug from previous versions. I am not sure who added it but do you see any reason for this to consider as an open item for PG-14? [1] - https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Mon, Aug 9, 2021 at 2:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > > > Please find enclosed a patch proposal to: > > > > * Avoid the failed assertion on current master and generate the error message instead (should the code reach that stage). > > * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro is working). > > > > I think instead of resetting toast_hash for this case why don't we set > 'relrewrite' for toast tables as well during rewrite? If we do that > then we will simply skip assembling toast chunks for the toast table. > In make_new_heap(), we are calling NewHeapCreateToastTable() to create > toast table where we can pass additional information (probably > 'toastid'), if required to set 'relrewrite'. Additionally, let's add a > test case if possible for this. I agree with Amit, that setting relrewrite for the toast relation as well is better as we can simply avoid processing the toast tuple as well in such cases. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi Amit, On 8/9/21 10:37 AM, Amit Kapila wrote: > On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >> Please find enclosed a patch proposal to: >> >> * Avoid the failed assertion on current master and generate the error message instead (should the code reach that stage). >> * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro is working). >> > I think instead of resetting toast_hash for this case why don't we set > 'relrewrite' for toast tables as well during rewrite? If we do that > then we will simply skip assembling toast chunks for the toast table. Thanks for looking at it! I do agree, that would be even better than the current patch approach: I'll work on it. > In make_new_heap(), we are calling NewHeapCreateToastTable() to create > toast table where we can pass additional information (probably > 'toastid'), if required to set 'relrewrite'. Additionally, let's add a > test case if possible for this. + 1 for the test case, it will be added in the next version of the patch. > > BTW, I see this as an Open Item for PG-14 [1] which seems wrong to me > as this is a bug from previous versions. I am not sure who added it Me neither. > but do you see any reason for this to consider as an open item for > PG-14? No, I don't see any reasons as this is a bug from previous versions. Thanks Bertrand
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > Hi Amit, > > On 8/9/21 10:37 AM, Amit Kapila wrote: > > On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > >> Please find enclosed a patch proposal to: > >> > >> * Avoid the failed assertion on current master and generate the error message instead (should the code reach that stage). > >> * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro is working). > >> > > I think instead of resetting toast_hash for this case why don't we set > > 'relrewrite' for toast tables as well during rewrite? If we do that > > then we will simply skip assembling toast chunks for the toast table. > > Thanks for looking at it! > > I do agree, that would be even better than the current patch approach: > I'll work on it. > > > In make_new_heap(), we are calling NewHeapCreateToastTable() to create > > toast table where we can pass additional information (probably > > 'toastid'), if required to set 'relrewrite'. Additionally, let's add a > > test case if possible for this. > + 1 for the test case, it will be added in the next version of the patch. > Thanks, please see, if you can prepare patches for the back-branches as well. -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > > BTW, I see this as an Open Item for PG-14 [1] which seems wrong to me > > as this is a bug from previous versions. I am not sure who added it > > Me neither. > > > but do you see any reason for this to consider as an open item for > > PG-14? > > No, I don't see any reasons as this is a bug from previous versions. > Thanks for the confirmation. I have moved this to the section: "Older bugs affecting stable branches". -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi Amit, On 8/9/21 1:12 PM, Amit Kapila wrote: > On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >> Hi Amit, >> >> On 8/9/21 10:37 AM, Amit Kapila wrote: >>> On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >>>> Please find enclosed a patch proposal to: >>>> >>>> * Avoid the failed assertion on current master and generate the error message instead (should the code reach that stage). >>>> * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro is working). >>>> >>> I think instead of resetting toast_hash for this case why don't we set >>> 'relrewrite' for toast tables as well during rewrite? If we do that >>> then we will simply skip assembling toast chunks for the toast table. >> Thanks for looking at it! >> >> I do agree, that would be even better than the current patch approach: >> I'll work on it. >> >>> In make_new_heap(), we are calling NewHeapCreateToastTable() to create >>> toast table where we can pass additional information (probably >>> 'toastid'), if required to set 'relrewrite'. Additionally, let's add a >>> test case if possible for this. >> + 1 for the test case, it will be added in the next version of the patch. >> > Thanks, please see, if you can prepare patches for the back-branches as well. Please find attached the new version that: - sets "relwrewrite" for the toast. - contains a new test case. As far preparing the patches for the back-branches: I will do it for sure, but I would prefer that we agree on a polished version on current master first. Thanks Bertrand
Attachment
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi Amit, On 8/10/21 1:59 PM, Drouvot, Bertrand wrote: > Hi Amit, > > On 8/9/21 1:12 PM, Amit Kapila wrote: >> On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand >> <bdrouvot@amazon.com> wrote: >>> Hi Amit, >>> >>> On 8/9/21 10:37 AM, Amit Kapila wrote: >>>> On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand >>>> <bdrouvot@amazon.com> wrote: >>>>> Please find enclosed a patch proposal to: >>>>> >>>>> * Avoid the failed assertion on current master and generate the >>>>> error message instead (should the code reach that stage). >>>>> * Reset the toast_hash in case of relation rewrite with toast (so >>>>> that the logical decoding in the above repro is working). >>>>> >>>> I think instead of resetting toast_hash for this case why don't we set >>>> 'relrewrite' for toast tables as well during rewrite? If we do that >>>> then we will simply skip assembling toast chunks for the toast table. >>> Thanks for looking at it! >>> >>> I do agree, that would be even better than the current patch approach: >>> I'll work on it. >>> >>>> In make_new_heap(), we are calling NewHeapCreateToastTable() to create >>>> toast table where we can pass additional information (probably >>>> 'toastid'), if required to set 'relrewrite'. Additionally, let's add a >>>> test case if possible for this. >>> + 1 for the test case, it will be added in the next version of the >>> patch. >>> >> Thanks, please see, if you can prepare patches for the back-branches >> as well. > > Please find attached the new version that: > > - sets "relwrewrite" for the toast. > > - contains a new test case. The first version of the patch contained a change in ReorderBufferToastReplace() (to put the call to RelationIsValid(toast_rel) and display the error message when it is not valid before the call to ReorderBufferChangeMemoryUpdate()). That way we also avoid the failed assertion described in the first message of this thread (but would report the error message instead). Forgot to mention that I did not add this change in the new patch version as I’m thinking it would be better to create another patch for that purpose (as not really related to toast rewrite), what do you think? Thanks Bertrand
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Thu, Aug 12, 2021 at 12:15 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > On 8/10/21 1:59 PM, Drouvot, Bertrand wrote: > > Hi Amit, > > > > The first version of the patch contained a change in > ReorderBufferToastReplace() (to put the call to > RelationIsValid(toast_rel) and display the error message when it is not > valid before the call to ReorderBufferChangeMemoryUpdate()). > > That way we also avoid the failed assertion described in the first > message of this thread (but would report the error message instead). > > Forgot to mention that I did not add this change in the new patch > version > I think that is the right decision. -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Tue, Aug 10, 2021 at 5:30 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > Hi Amit, > > On 8/9/21 1:12 PM, Amit Kapila wrote: > > On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > >> Hi Amit, > >> > >> On 8/9/21 10:37 AM, Amit Kapila wrote: > >>> On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > >>>> Please find enclosed a patch proposal to: > >>>> > >>>> * Avoid the failed assertion on current master and generate the error message instead (should the code reach thatstage). > >>>> * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro isworking). > >>>> > >>> I think instead of resetting toast_hash for this case why don't we set > >>> 'relrewrite' for toast tables as well during rewrite? If we do that > >>> then we will simply skip assembling toast chunks for the toast table. > >> Thanks for looking at it! > >> > >> I do agree, that would be even better than the current patch approach: > >> I'll work on it. > >> > >>> In make_new_heap(), we are calling NewHeapCreateToastTable() to create > >>> toast table where we can pass additional information (probably > >>> 'toastid'), if required to set 'relrewrite'. Additionally, let's add a > >>> test case if possible for this. > >> + 1 for the test case, it will be added in the next version of the patch. > >> > > Thanks, please see, if you can prepare patches for the back-branches as well. > > Please find attached the new version that: > > - sets "relwrewrite" for the toast. > --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bo */ namestrcpy(&(relform->relname), newrelname); + /* reset relrewrite for toast */ + if (relform->relkind == RELKIND_TOASTVALUE) + relform->relrewrite = InvalidOid; + I find this change quite ad-hoc. I think this API is quite generic to make such a change. I see two ways for this (a) pass a new bool flag (reset_toast_rewrite) in this API and then make this change, (b) in the specific place where we need this, change relrewrite separately via a new API. I would prefer (b) in the ideal case, but I understand it would be an additional cost, so maybe (a) is also okay. What do you people think? -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Thu, Aug 12, 2021 at 4:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 10, 2021 at 5:30 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > > > > > Please find attached the new version that: > > > > - sets "relwrewrite" for the toast. > > > > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char > *newrelname, bool is_internal, bo > */ > namestrcpy(&(relform->relname), newrelname); > > + /* reset relrewrite for toast */ > + if (relform->relkind == RELKIND_TOASTVALUE) > + relform->relrewrite = InvalidOid; > + > > I find this change quite ad-hoc. I think this API is quite generic to > make such a change. I see two ways for this (a) pass a new bool flag > (reset_toast_rewrite) in this API and then make this change, (b) in > the specific place where we need this, change relrewrite separately > via a new API. > > I would prefer (b) in the ideal case, but I understand it would be an > additional cost, so maybe (a) is also okay. What do you people think? > One minor comment: +/* + * Test decoding relation rewrite with toast. + * The insert into tbl2 within the same transaction + * is there to check there is no remaining toast_hash + * not being reset. + */ You can extend each line of comment up to 80 chars. The current one looks a bit odd. -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi, On 8/12/21 1:00 PM, Amit Kapila wrote: > On Tue, Aug 10, 2021 at 5:30 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >> Hi Amit, >> >> On 8/9/21 1:12 PM, Amit Kapila wrote: >>> On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >>>> Hi Amit, >>>> >>>> On 8/9/21 10:37 AM, Amit Kapila wrote: >>>>> On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >>>>>> Please find enclosed a patch proposal to: >>>>>> >>>>>> * Avoid the failed assertion on current master and generate the error message instead (should the code reach thatstage). >>>>>> * Reset the toast_hash in case of relation rewrite with toast (so that the logical decoding in the above repro isworking). >>>>>> >>>>> I think instead of resetting toast_hash for this case why don't we set >>>>> 'relrewrite' for toast tables as well during rewrite? If we do that >>>>> then we will simply skip assembling toast chunks for the toast table. >>>> Thanks for looking at it! >>>> >>>> I do agree, that would be even better than the current patch approach: >>>> I'll work on it. >>>> >>>>> In make_new_heap(), we are calling NewHeapCreateToastTable() to create >>>>> toast table where we can pass additional information (probably >>>>> 'toastid'), if required to set 'relrewrite'. Additionally, let's add a >>>>> test case if possible for this. >>>> + 1 for the test case, it will be added in the next version of the patch. >>>> >>> Thanks, please see, if you can prepare patches for the back-branches as well. >> Please find attached the new version that: >> >> - sets "relwrewrite" for the toast. >> > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char > *newrelname, bool is_internal, bo > */ > namestrcpy(&(relform->relname), newrelname); > > + /* reset relrewrite for toast */ > + if (relform->relkind == RELKIND_TOASTVALUE) > + relform->relrewrite = InvalidOid; > + > > I find this change quite ad-hoc. I think this API is quite generic to > make such a change. I see two ways for this (a) pass a new bool flag > (reset_toast_rewrite) in this API and then make this change, (b) in > the specific place where we need this, change relrewrite separately > via a new API. > > I would prefer (b) in the ideal case, but I understand it would be an > additional cost, so maybe (a) is also okay. What do you people think? I would prefer a) because: - b) would need to update the exact same tuple one more time (means doing more or less the same work: open relation, search for the tuple, update the tuple...) - a) would still give the ability for someone reading the code to understand where the relrewrite reset is needed (as opposed to the way the patch is currently written) - finish_heap_swap() with swap_toast_by_content set to false, is the only place where we currently need to reset explicitly relrewrite (so that we would have the new API produced by b) being called only at that place). - That means that b) would be only for code readability but at the price of extra cost. That said, I think we can go with a) and rethink about b) later on if there is a need of this new API in other places for other reasons. Thanks Bertrand
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On 8/12/21 1:28 PM, Amit Kapila wrote: > On Thu, Aug 12, 2021 at 4:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Tue, Aug 10, 2021 at 5:30 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >>> >>> Please find attached the new version that: >>> >>> - sets "relwrewrite" for the toast. >>> >> --- a/src/backend/commands/tablecmds.c >> +++ b/src/backend/commands/tablecmds.c >> @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char >> *newrelname, bool is_internal, bo >> */ >> namestrcpy(&(relform->relname), newrelname); >> >> + /* reset relrewrite for toast */ >> + if (relform->relkind == RELKIND_TOASTVALUE) >> + relform->relrewrite = InvalidOid; >> + >> >> I find this change quite ad-hoc. I think this API is quite generic to >> make such a change. I see two ways for this (a) pass a new bool flag >> (reset_toast_rewrite) in this API and then make this change, (b) in >> the specific place where we need this, change relrewrite separately >> via a new API. >> >> I would prefer (b) in the ideal case, but I understand it would be an >> additional cost, so maybe (a) is also okay. What do you people think? >> > One minor comment: > +/* > + * Test decoding relation rewrite with toast. > + * The insert into tbl2 within the same transaction > + * is there to check there is no remaining toast_hash > + * not being reset. > + */ > > You can extend each line of comment up to 80 chars. The current one > looks a bit odd. Thanks. I'll update the patch and comment that way once we have decided if we are going for a) or b). Thanks Bertrand
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Fri, Aug 13, 2021 at 11:47 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > On 8/12/21 1:00 PM, Amit Kapila wrote: > >> > >> - sets "relwrewrite" for the toast. > >> > > --- a/src/backend/commands/tablecmds.c > > +++ b/src/backend/commands/tablecmds.c > > @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char > > *newrelname, bool is_internal, bo > > */ > > namestrcpy(&(relform->relname), newrelname); > > > > + /* reset relrewrite for toast */ > > + if (relform->relkind == RELKIND_TOASTVALUE) > > + relform->relrewrite = InvalidOid; > > + > > > > I find this change quite ad-hoc. I think this API is quite generic to > > make such a change. I see two ways for this (a) pass a new bool flag > > (reset_toast_rewrite) in this API and then make this change, (b) in > > the specific place where we need this, change relrewrite separately > > via a new API. > > > > I would prefer (b) in the ideal case, but I understand it would be an > > additional cost, so maybe (a) is also okay. What do you people think? > > I would prefer a) because: > > - b) would need to update the exact same tuple one more time (means > doing more or less the same work: open relation, search for the tuple, > update the tuple...) > > - a) would still give the ability for someone reading the code to > understand where the relrewrite reset is needed (as opposed to the way > the patch is currently written) > > - finish_heap_swap() with swap_toast_by_content set to false, is the > only place where we currently need to reset explicitly relrewrite (so > that we would have the new API produced by b) being called only at that > place). > > - That means that b) would be only for code readability but at the price > of extra cost. > Anybody else would like to weigh in? I think it is good if few others also share their opinion as we need to backpatch this bug-fix. -- With Regards, Amit Kapila.
Re: [UNVERIFIED SENDER] Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi Drouvot, > I don't see extra data in your output and it looks like your > copy/paste is missing some content, no? > > On my side, that looks good and here is what i get with the patch applied: > I ran the test again, now I got the same output as yours, and it looks good for me. (The issue I mentioned in previous email was caused by my console output.) Thank you, -- David Software Engineer Highgo Software Inc. (Canada) www.highgo.ca
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi, On 8/13/21 11:17 AM, Amit Kapila wrote: > On Fri, Aug 13, 2021 at 11:47 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >> On 8/12/21 1:00 PM, Amit Kapila wrote: >>>> - sets "relwrewrite" for the toast. >>>> >>> --- a/src/backend/commands/tablecmds.c >>> +++ b/src/backend/commands/tablecmds.c >>> @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char >>> *newrelname, bool is_internal, bo >>> */ >>> namestrcpy(&(relform->relname), newrelname); >>> >>> + /* reset relrewrite for toast */ >>> + if (relform->relkind == RELKIND_TOASTVALUE) >>> + relform->relrewrite = InvalidOid; >>> + >>> >>> I find this change quite ad-hoc. I think this API is quite generic to >>> make such a change. I see two ways for this (a) pass a new bool flag >>> (reset_toast_rewrite) in this API and then make this change, (b) in >>> the specific place where we need this, change relrewrite separately >>> via a new API. >>> >>> I would prefer (b) in the ideal case, but I understand it would be an >>> additional cost, so maybe (a) is also okay. What do you people think? >> I would prefer a) because: >> >> - b) would need to update the exact same tuple one more time (means >> doing more or less the same work: open relation, search for the tuple, >> update the tuple...) >> >> - a) would still give the ability for someone reading the code to >> understand where the relrewrite reset is needed (as opposed to the way >> the patch is currently written) >> >> - finish_heap_swap() with swap_toast_by_content set to false, is the >> only place where we currently need to reset explicitly relrewrite (so >> that we would have the new API produced by b) being called only at that >> place). >> >> - That means that b) would be only for code readability but at the price >> of extra cost. >> > Anybody else would like to weigh in? I think it is good if few others > also share their opinion as we need to backpatch this bug-fix. I had a second thoughts on it and now think option b) is better. It would make the code clearer by using a new API and the extra cost of it (mainly search again for the pg_class tuple and update it) should be ok. Please find patch version V3 making use of a new API. Thanks Bertrand
Attachment
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > Hi, > > On 8/13/21 11:17 AM, Amit Kapila wrote: > > On Fri, Aug 13, 2021 at 11:47 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > >> On 8/12/21 1:00 PM, Amit Kapila wrote: > >>>> - sets "relwrewrite" for the toast. > >>>> > >>> --- a/src/backend/commands/tablecmds.c > >>> +++ b/src/backend/commands/tablecmds.c > >>> @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char > >>> *newrelname, bool is_internal, bo > >>> */ > >>> namestrcpy(&(relform->relname), newrelname); > >>> > >>> + /* reset relrewrite for toast */ > >>> + if (relform->relkind == RELKIND_TOASTVALUE) > >>> + relform->relrewrite = InvalidOid; > >>> + > >>> > >>> I find this change quite ad-hoc. I think this API is quite generic to > >>> make such a change. I see two ways for this (a) pass a new bool flag > >>> (reset_toast_rewrite) in this API and then make this change, (b) in > >>> the specific place where we need this, change relrewrite separately > >>> via a new API. > >>> > >>> I would prefer (b) in the ideal case, but I understand it would be an > >>> additional cost, so maybe (a) is also okay. What do you people think? > >> I would prefer a) because: > >> > >> - b) would need to update the exact same tuple one more time (means > >> doing more or less the same work: open relation, search for the tuple, > >> update the tuple...) > >> > >> - a) would still give the ability for someone reading the code to > >> understand where the relrewrite reset is needed (as opposed to the way > >> the patch is currently written) > >> > >> - finish_heap_swap() with swap_toast_by_content set to false, is the > >> only place where we currently need to reset explicitly relrewrite (so > >> that we would have the new API produced by b) being called only at that > >> place). > >> > >> - That means that b) would be only for code readability but at the price > >> of extra cost. > >> > > Anybody else would like to weigh in? I think it is good if few others > > also share their opinion as we need to backpatch this bug-fix. > > I had a second thoughts on it and now think option b) is better. > > It would make the code clearer by using a new API and the extra cost of > it (mainly search again for the pg_class tuple and update it) should be ok. > I agree especially because I am not very comfortable changing the RenameRelationInternal() API in back branches. One minor comment: + + /* + * Reset the relrewrite for the toast. We need to call + * CommandCounterIncrement() first to avoid the + * "tuple already updated by self" error. Indeed the exact same + * pg_class tuple has already been updated while + * calling RenameRelationInternal(). + */ + CommandCounterIncrement(); It would be better if we can write the above comment as "The command-counter increment is required here as we are about to update the tuple that is updated as part of RenameRelationInternal." or something like that. I would like to push and back-patch the proposed patch (after some minor edits in the comments) unless someone thinks otherwise. -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi, On 8/18/21 12:01 PM, Amit Kapila wrote: > On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >> Hi, >> >> On 8/13/21 11:17 AM, Amit Kapila wrote: >>> On Fri, Aug 13, 2021 at 11:47 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >>>> On 8/12/21 1:00 PM, Amit Kapila wrote: >>>>>> - sets "relwrewrite" for the toast. >>>>>> >>>>> --- a/src/backend/commands/tablecmds.c >>>>> +++ b/src/backend/commands/tablecmds.c >>>>> @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char >>>>> *newrelname, bool is_internal, bo >>>>> */ >>>>> namestrcpy(&(relform->relname), newrelname); >>>>> >>>>> + /* reset relrewrite for toast */ >>>>> + if (relform->relkind == RELKIND_TOASTVALUE) >>>>> + relform->relrewrite = InvalidOid; >>>>> + >>>>> >>>>> I find this change quite ad-hoc. I think this API is quite generic to >>>>> make such a change. I see two ways for this (a) pass a new bool flag >>>>> (reset_toast_rewrite) in this API and then make this change, (b) in >>>>> the specific place where we need this, change relrewrite separately >>>>> via a new API. >>>>> >>>>> I would prefer (b) in the ideal case, but I understand it would be an >>>>> additional cost, so maybe (a) is also okay. What do you people think? >>>> I would prefer a) because: >>>> >>>> - b) would need to update the exact same tuple one more time (means >>>> doing more or less the same work: open relation, search for the tuple, >>>> update the tuple...) >>>> >>>> - a) would still give the ability for someone reading the code to >>>> understand where the relrewrite reset is needed (as opposed to the way >>>> the patch is currently written) >>>> >>>> - finish_heap_swap() with swap_toast_by_content set to false, is the >>>> only place where we currently need to reset explicitly relrewrite (so >>>> that we would have the new API produced by b) being called only at that >>>> place). >>>> >>>> - That means that b) would be only for code readability but at the price >>>> of extra cost. >>>> >>> Anybody else would like to weigh in? I think it is good if few others >>> also share their opinion as we need to backpatch this bug-fix. >> I had a second thoughts on it and now think option b) is better. >> >> It would make the code clearer by using a new API and the extra cost of >> it (mainly search again for the pg_class tuple and update it) should be ok. >> > I agree especially because I am not very comfortable changing the > RenameRelationInternal() API in back branches. One minor comment: > > + > + /* > + * Reset the relrewrite for the toast. We need to call > + * CommandCounterIncrement() first to avoid the > + * "tuple already updated by self" error. Indeed the exact same > + * pg_class tuple has already been updated while > + * calling RenameRelationInternal(). > + */ > + CommandCounterIncrement(); > > It would be better if we can write the above comment as "The > command-counter increment is required here as we are about to update > the tuple that is updated as part of RenameRelationInternal." or > something like that. > > I would like to push and back-patch the proposed patch (after some > minor edits in the comments) unless someone thinks otherwise. Thanks! I've updated the comment and prepared the back patch versions: Please find attached: v4-0001-toast-rewrite-master-branch.patch: to be applied on the master and REL_14_STABLE branches v4-0001-toast-rewrite-13-stable-branch.patch: to be applied on the REL_13_STABLE and REL_12_STABLE branches v4-0001-toast-rewrite-11-stable-branch.patch: to be applied on the REL_11_STABLE branch I stopped the back patching here as the issue has been introduced in 325f2ec555. Thanks Bertrand
Attachment
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Wed, Aug 18, 2021 at 8:09 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > Hi, > > On 8/18/21 12:01 PM, Amit Kapila wrote: > > On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > >> Hi, > > I've updated the comment and prepared the back patch versions: > I have verified and all your patches look good to me. I'll push and backpatch this by Wednesday unless there are more comments. -- With Regards, Amit Kapila.
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
Hi, On 8/23/21 12:02 PM, Amit Kapila wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you canconfirm the sender and know the content is safe. > > > > On Wed, Aug 18, 2021 at 8:09 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >> Hi, >> >> On 8/18/21 12:01 PM, Amit Kapila wrote: >>> On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: >>>> Hi, >> I've updated the comment and prepared the back patch versions: >> > I have verified and all your patches look good to me. I'll push and > backpatch this by Wednesday unless there are more comments. I just saw that the patch has been committed. Thanks for your help and time on this. I'll mark the corresponding commitfest entry as "Committed". Thanks Bertrand
Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash
On Wed, Aug 25, 2021 at 11:26 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote: > > I just saw that the patch has been committed. > > Thanks for your help and time on this. > > I'll mark the corresponding commitfest entry as "Committed". > Thanks for your work on this. I have also marked it closed in PostgreSQL_14_Open_Items doc [1] [1] - https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items -- With Regards, Amit Kapila.