RE: Selectively invalidate caches in pgoutput module

On Monday, March 10, 2025 9:12 PM Kuroda, Hayato <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> > Currently, only the leaf partition is invalidated when the published
> > table is partitioned. However, I think pgoutput could cache both the
> > partitioned table and the leaf partition table as relsync entries.
> >
> > For INSERT/UPDATE/DELETE on a partitioned table, only the leaf
> > partition's relsync entry is used in pgoutput, but the TRUNCATE
> > references the parent table's relsync entry.
>
> I think your analysis is correct. PSA new version. Below part contains my
> analysis.
>
> In ExecuteTruncate(), if the specified relation has children, all of them are
> checked via find_all_inheritors() and listed as target. Also,
> ExecuteTruncateGuts() serializes both a parent and children in
> XLOG_HEAP_TRUNCATE WAL record.
> Decoding layer passes relations as-is. These facts mean that output plugins
> can store caches on the memory.

Thanks for updating the patch.

I have reviewed patch 0001 and did not find issues, aside from a few issues for
code comments that were mentioned in Amit's email. Here are some analyses and
insights gathered during the review of 0001:

The functions and variables in the 0001 patch uses 'Relsync' (e.g.,
RegisterRelsyncInvalidation) instead of the longer 'RelsyncCache'. After
internal discussions, we think it's OK, as using 'RelsyncCache' could
unnecessarily lengthen the names.

Furthermore, considering we're introducing a new invalidation message for the
RelationSyncCache within pgoutput, which is an output plugin, we discussed
whether a more general invalidation name should be adopted in case other output
plugins might use it. However, after reviewing all the plugins listed in
Wiki[1], we did not find any other plugins that reference the built-in
publication catalog. Therefore, this scenario appears to be uncommon.
Additionally, the current naming convention is sufficiently intuitive for
output plugin developers. Hence, we feel it is reasonable to retain the
existing names.

For patch 0002, I think the implementation could be improved. The
current patch introduces a new function, RenamePublication, to replace the
existing generic approach in ExecRenameStmt->AlterObjectRename_internal.
However, this creates inconsistency because the original code uses
AccessExclusiveLock for publication renaming, making concurrent renaming
impossible. The current patch seems to overlook this aspect.

Additionally, introducing a new function results in code duplication, which can
be avoided. After further consideration, handling the publication rename
separately seems unnecessary, given it requires only sending a few extra
invalidation messages. Therefore, I suggest reusing the generic handling and
simply extending AlterObjectRename_internal to include the additional
invalidation messages.

I've attached a diff with the proposed changes for 0002.

Best Regards,
Hou zj