Re: row filtering for logical replication

On Wed, Jul 14, 2021 at 6:38 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> Peter, thanks for quickly check the new patch. I'm attaching a new patch (v19)
> that addresses (a) this new review, (b) Tomas' review and (c) Greg's review. I
> also included the copy/equal node support for the new node (PublicationTable)
> mentioned by Tomas in another email.
>

Some minor v19 patch review points you might consider for your next
patch version:
(I'm still considering the other issues raised about WHERE clauses and
filtering)

(1) src/backend/commands/publicationcmds.c
OpenTableList

Some suggested abbreviations:

BEFORE:
if (IsA(lfirst(lc), PublicationTable))
whereclause = true;
else
whereclause = false;

AFTER:
whereclause = IsA(lfirst(lc), PublicationTable);

BEFORE:
if (whereclause)
pri->whereClause = t->whereClause;
else
pri->whereClause = NULL;

AFTER:
pri->whereClause = whereclause? t->whereClause : NULL;

(2) src/backend/parser/parse_expr.c

I think that the check below:

/* Functions are not allowed in publication WHERE clauses */
if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE &&
nodeTag(expr) == T_FuncCall)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("functions are not allowed in publication WHERE expressions"),
parser_errposition(pstate, exprLocation(expr))));

should be moved down into the "T_FuncCall" case of the switch
statement below it, so that "if (pstate->p_expr_kind ==
EXPR_KIND_PUBLICATION_WHERE" doesn't get checked every call to
transformExprRecurse() regardless of the expression Node type.

(3) Save a nanosecond when entry->exprstate is already NIL:

BEFORE:
if (entry->exprstate != NIL)
list_free_deep(entry->exprstate);
entry->exprstate = NIL;

AFTER:
if (entry->exprstate != NIL)
{
list_free_deep(entry->exprstate);
entry->exprstate = NIL;
}

Regards,
Greg Nancarrow
Fujitsu Australia