|From:||Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>|
|To:||Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>|
|Cc:||Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Euler Taveira <euler(at)eulerto(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Petr Jelinek <petr(dot)jelinek(at)enterprisedb(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [BUG]Update Toast data failure in logical replication|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
I don't have a reason not to commit this patch. I have some suggestions
on the comments and docs though.
> @@ -8359,14 +8408,15 @@ log_heap_new_cid(Relation relation, HeapTuple tup)
> * Returns NULL if there's no need to log an identity or if there's no suitable
> * key defined.
> - * key_changed should be false if caller knows that no replica identity
> - * columns changed value. It's always true in the DELETE case.
> + * key_required should be false if caller knows that no replica identity
> + * columns changed value and it doesn't has any external data. It's always
> + * true in the DELETE case.
> * *copy is set to true if the returned tuple is a modified copy rather than
> * the same tuple that was passed in.
> static HeapTuple
> -ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed,
> +ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_required,
I find the new comment pretty hard to interpret. I would say something
like "Pass key_required true if any replica identity columns changed
value, or if any of them have external data. DELETE must always pass
> diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
> index dee026e..d67ef7c 100644
> --- a/doc/src/sgml/ref/alter_table.sgml
> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -873,8 +873,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
> This form changes the information which is written to the write-ahead log
> to identify rows which are updated or deleted. This option has no effect
> except when logical replication is in use.
> - In all cases, no old values are logged unless at least one of the columns
> - that would be logged differs between the old and new versions of the row.
> + In all cases except toasted values, no old values are logged unless at
> + least one of the columns that would be logged differs between the old and
> + new versions of the row. We detoast the unchanged old toast values and log
> + them.
Here we're patching with a minimal wording change with almost
incomprehensible results. I think we should patch more extensively.
This form changes the information which is written to the
write-ahead log to identify rows which are updated or deleted.
In most cases, the old value of each column is only logged if
it differs from the new value; however, if the old value is
stored externally, it is always logged regardless of whether it
This option has no effect unless logical replication is in use.
I didn't get a chance to review the code, but I think this is valuable.
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
|Next Message||Laurenz Albe||2022-02-04 16:09:44||Re: [PATCH] Add reloption for views to enable RLS|
|Previous Message||Fujii Masao||2022-02-04 15:30:26||Re: [PATCH] Add min() and max() aggregate functions for xid8|