[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

Hi David,

On 8/7/21 12:18 AM, David Zhang wrote:

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

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]https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items -- With Regards, Amit Kapila. 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.
--
With Regards,
Amit Kapila.

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

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

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.

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.

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

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

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.

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 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.

--- 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.

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.

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 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.

--- 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

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

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.

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

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

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.

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

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.

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 can confirm 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

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]https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items

[1]: https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items

--
With Regards,
Amit Kapila.