Re: row filtering for logical replication

On Wed, Jul 14, 2021 at 6:38 AM Euler Taveira  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