Re: row filtering for logical replication

On Tue, Oct 12, 2021 at 1:37 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Oct 6, 2021 at 2:33 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Sat, Oct 2, 2021 at 5:44 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
> > > I have for now also rebased the patch and merged the first 5 patches
> > > into 1, and added my changes for the above into the second patch.
> >
> > I have split the patches back again, just to be consistent with the
> > original state of the patches. Sorry for the inconvenience.
>
> Thanks for the updated version of the patch, I was looking into the
> latest version and I have a few comments.
>
>
> + 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]))))
> + {
> + tmp_new_slot->tts_values[i] = old_slot->tts_values[i];
> + newtup_changed = true;
> + }
>
> If the attribute is stored EXTERNAL_ONDIS on the new tuple and it is
> not null in the old tuple then it must be logged completely in the old
> tuple, so instead of checking
> !(VARATT_IS_EXTERNAL_ONDISK(old_slot->tts_values[i]), it should be
> asserted,
>
>
> + heap_deform_tuple(newtuple, desc, new_slot->tts_values,
> new_slot->tts_isnull);
> + heap_deform_tuple(oldtuple, desc, old_slot->tts_values,
> old_slot->tts_isnull);
> +
> + if (newtup_changed)
> + tmpnewtuple = heap_form_tuple(desc, tmp_new_slot->tts_values,
> new_slot->tts_isnull);
> +
> + old_matched = pgoutput_row_filter(relation, NULL, oldtuple, entry);
> + new_matched = pgoutput_row_filter(relation, NULL,
> + newtup_changed ? tmpnewtuple :
> newtuple, entry);
>
> I do not like the fact that, first we have deformed the tuples and we
> are again using the HeapTuple
> for expression evaluation machinery and later the expression
> evaluation we do the deform again.
>
> So why don't you use the deformed tuple as it is to store as a virtual tuple?
>
> Infact, if newtup_changed is true then you are forming back the tuple
> just to get it deformed again
> in the expression evaluation.
>
> I think I have already given this comment on the last version.

Right, I only used the deformed tuple later when it was written to the
stream. I will modify this as well.

regards,
Ajin Cherian
Fujitsu Australia