Re: row filtering for logical replication

Hi,

I took a look at this patch, which seems to be in CF since 2018. I have
only some basic comments and observations at this point:

1) alter_publication.sgml

I think "expression is executed" sounds a bit strange, perhaps
"evaluated" would be better?

2) create_publication.sgml

Why is the patch changing publish_via_partition_root docs? That seems
like a rather unrelated bit.

The <literal>WHERE</literal> clause should probably contain only
columns that are part of the primary key or be covered by
<literal>REPLICA ...

I'm not sure what exactly is this trying to say. What does "should
probably ..." mean in practice for the users? Does that mean something
bad will happen for other columns, or what? I'm sure this wording will
be quite confusing for users.

It may also be unclear whether the condition is evaluated on the old or
new row, so perhaps add an example illustrating that & more detailed
comment, or something. E.g. what will happen with

UPDATE departments SET active = false WHERE active;

3) publication_add_relation

Does this need to build the parse state even for whereClause == NULL?

4) AlterPublicationTables

I wonder if this new reworked code might have issues with subscriptions
containing many tables, but I haven't tried.

5) OpenTableList

I really dislike that the list can have two different node types
(Relation and PublicationTable). In principle we don't actually need the
extra flag, we can simply check the node type directly by IsA() and act
based on that. However, I think it'd be better to just use a single node
type from all places.

I don't see why not to set whereClause every time, I don't think the
extra if saves anything, it's just a bit more complex.

5) CloseTableList

The comment about node types seems pointless, this function has no flag
and the element type does not matter.

6) parse_agg.c

... are not allowed in publication WHERE expressions

I think all similar cases use "WHERE conditions" instead.

7) transformExprRecurse

The check at the beginning seems rather awkward / misplaced - it's way
too specific for this location (there are no other p_expr_kind
references in this function). Wouldn't transformFuncCall (or maybe
ParseFuncOrColumn) be a more appropriate place?

Initially I was wondering why not to allow function calls in WHERE
conditions, but I see that was discussed in the past as problematic. But
that reminds me that I don't see any docs describing what expressions
are allowed in WHERE conditions - maybe we should explicitly list what
expressions are allowed?

8) pgoutput.c

I have not reviewed this in detail yet, but there seems to be something
wrong because `make check-world` fails in subscription/010_truncate.pl
after hitting an assert (backtrace attached) during "START_REPLICATION
SLOT" in get_rel_sync_entry in this code:

/* Release tuple table slot */
if (entry->scantuple != NULL)
{
ExecDropSingleTupleTableSlot(entry->scantuple);
entry->scantuple = NULL;
}

So there seems to be something wrong with how the slot is created.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company