Horton

Re: [BUG]Update Toast data failure in logical replication

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, Petr Jelinek <petr.jelinek@enterprisedb.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Tomas Vondra <tomas.vondra@enterprisedb.com>, Andres Freund <andres@anarazel.de>, Kuntal Ghosh <kuntalghosh.2007@gmail.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-02-04T15:36:19Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. WAL log unchanged toasted replica identity key attributes.

  2. Add new wal_level, logical, sufficient for logical decoding.

  3. Add the notion of REPLICA IDENTITY for a table.

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

> 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.
I suggest:

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

	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/