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
Program terminated with signal SIGABRT, Aborted.
#0 0x00007e8ed2ec29e5 in ?? () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install
glibc-2.32-8.fc33.x86_64 libgcc-10.3.1-1.fc33.x86_64
(gdb) bt
#0 0x00007e8ed2ec29e5 in ?? () from /lib64/libc.so.6
#1 0x00007e8ed2eab8a4 in ?? () from /lib64/libc.so.6
#2 0x0000000000afffe1 in ExceptionalCondition (conditionName=0xb7f8e5
"tupdesc->tdrefcount > 0", errorType=0xb7f8a3 "FailedAssertion",
fileName=0xb7f810 "tupdesc.c", lineNumber=386) at assert.c:69
#3 0x00000000004a55cb in DecrTupleDescRefCount (tupdesc=0x7e8ed23b4578) at
tupdesc.c:386
#4 0x000000000073fd84 in ExecDropSingleTupleTableSlot (slot=0x7e8ed23b8910) at
execTuples.c:1261
#5 0x00007e8ed23f2aed in get_rel_sync_entry (data=0x2875440,
relation=0x7e8ed23b86c0) at pgoutput.c:1258
#6 0x00007e8ed23f20f8 in pgoutput_truncate (ctx=0x286cc70, txn=0x2888c48,
nrelations=1, relations=0x2892dd8, change=0x288b138) at pgoutput.c:890
#7 0x00000000008cf03b in truncate_cb_wrapper (cache=0x286ec80, txn=0x2888c48,
nrelations=1, relations=0x2892dd8, change=0x288b138) at logical.c:1083
#8 0x00000000008d9605 in ReorderBufferApplyTruncate (rb=0x286ec80,
txn=0x2888c48, nrelations=1, relations=0x2892dd8, change=0x288b138,
streaming=false) at reorderbuffer.c:1928
#9 0x00000000008da08c in ReorderBufferProcessTXN (rb=0x286ec80, txn=0x2888c48,
commit_lsn=23903752, snapshot_now=0x286f168, command_id=2, streaming=false) at
reorderbuffer.c:2314
#10 0x00000000008da845 in ReorderBufferReplay (txn=0x2888c48, rb=0x286ec80,
xid=744, commit_lsn=23903752, end_lsn=23903960, commit_time=679358346883121,
origin_id=0, origin_lsn=0) at reorderbuffer.c:2619
#11 0x00000000008da8c3 in ReorderBufferCommit (rb=0x286ec80, xid=744,
commit_lsn=23903752, end_lsn=23903960, commit_time=679358346883121,
origin_id=0, origin_lsn=0) at reorderbuffer.c:2643
#12 0x00000000008ca72a in DecodeCommit (ctx=0x286cc70, buf=0x7fffd45cde20,
parsed=0x7fffd45cdc80, xid=744, two_phase=false) at decode.c:744
#13 0x00000000008c9a97 in DecodeXactOp (ctx=0x286cc70, buf=0x7fffd45cde20) at
decode.c:278
#14 0x00000000008c96c3 in LogicalDecodingProcessRecord (ctx=0x286cc70,
record=0x286d070) at decode.c:142
#15 0x00000000009024f7 in XLogSendLogical () at walsender.c:2884
#16 0x0000000000901872 in WalSndLoop (send_data=0x90245b <XLogSendLogical>) at
walsender.c:2314
#17 0x00000000009002b0 in StartLogicalReplication (cmd=0x28388c8) at
walsender.c:1214
#18 0x0000000000900beb in exec_replication_command (cmd_string=0x279edb0
"START_REPLICATION SLOT \"sub1\" LOGICAL 0/0 (proto_version '2',
publication_names '\"pub1\"')") at walsender.c:1654
#19 0x00000000009706b2 in PostgresMain (argc=1, argv=0x7fffd45ce140,
dbname=0x27cb818 "postgres", username=0x27cb7f8 "user") at postgres.c:4482
#20 0x00000000008ad219 in BackendRun (port=0x27c48d0) at postmaster.c:4507
#21 0x00000000008acb98 in BackendStartup (port=0x27c48d0) at postmaster.c:4229
#22 0x00000000008a914c in ServerLoop () at postmaster.c:1745
#23 0x00000000008a8a17 in PostmasterMain (argc=4, argv=0x2798800) at
postmaster.c:1417
#24 0x00000000007ab43c in main (argc=4, argv=0x2798800) at main.c:209
