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/
