Re: row filtering for logical replication

On Mon, Nov 15, 2021 at 8:14 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Nov 12, 2021 at 3:49 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > Attaching version 39-
> >
>
> Some comments on 0006
>
> --
...
> --
>
> Looking further, I realized that "logicalrep_write_tuple" and
> "logicalrep_write_tuple_cached" are completely duplicate except first
> one is calling "heap_deform_tuple" and then using local values[] array
> and the second one is directly using the slot->values[] array, so in
> fact we can pass this also as a parameter or we can put just one if
> check the populate the values[] and null array, so if it is cached we
> will point directly to the slot->values[] otherwise
> heap_deform_tuple(), I think this should be just one simple check.

Fixed in v40 [1]

> --
> +
> +/*
> + * Change is checked against the row filter, if any.
> + *
> + * If it returns true, the change is replicated, otherwise, it is not.
> + */
> +static bool
> +pgoutput_row_filter_virtual(Relation relation, TupleTableSlot *slot,
> RelationSyncEntry *entry)
>
> IMHO, the comments should explain how it is different from the
> pgoutput_row_filter function. Also comments are saying "If it returns
> true, the change is replicated, otherwise, it is not" which is not
> exactly true for this function, I mean based on that the caller will
> change the action. So I think it is enough to say what this function
> is doing but not required to say what the caller will do based on what
> this function returns.

Fixed in v40 [1].

>
>
> --
>
> + for (i = 0; i < desc->natts; i++)
> + {
> + Form_pg_attribute att = TupleDescAttr(desc, i);
> +
> + /* if the column in the new_tuple is null, nothing to do */
> + if (tmp_new_slot->tts_isnull[i])
> + continue;
>
> Put some comments over this loop about what it is trying to do, and
> overall I think there are not sufficient comments in the
> pgoutput_row_filter_update_check function.

Fixed in v40 [1].

>
> --
> + /*
> + * Unchanged toasted replica identity columns are
> + * only detoasted in the old tuple, copy this over to the newtuple.
> + */
> + if ((att->attlen == -1 &&
> VARATT_IS_EXTERNAL_ONDISK(tmp_new_slot->tts_values[i])) &&
> + (!old_slot->tts_isnull[i] &&
> + !(VARATT_IS_EXTERNAL_ONDISK(old_slot->tts_values[i]))))
>
> Is it ever possible that if the attribute is not NULL in the old slot
> still it is stored as VARATT_IS_EXTERNAL_ONDISK? I think no, so
> instead of adding
> this last condition in check it should be asserted inside the if check.
>

Fixed in v40 [1]

-----
[1] https://www.postgresql.org/message-id/CAHut%2BPv-D4rQseRO_OzfEz2dQsTKEnKjBCET9Z-iJppyT1XNMQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia