Re: Table refer leak in logical replication

On Wed, Apr 21, 2021 at 09:58:10PM +0900, Amit Langote wrote:
> Okay, done.

So, I have been working on that today, and tried to apply the full set
before realizing when writing the commit message that this was a set
of bullet points, and that this was too much for a single commit. The
tests are a nice thing to have to improve the coverage related to
tuple routing, but that these are not really mandatory for the sake of
the fix discussed here. So for now I have applied the main fix as of
f3b141c to close the open item.

Now.. Coming back to the tests.

- RETURN NULL;
+ IF (NEW.bid = 4 AND NEW.id = OLD.id) THEN
+ RETURN NEW;
+ ELSE
+ RETURN NULL;
+ END IF
This part added in test 003 is subtle. This is a tweak to make sure
that the second trigger, AFTER trigger added in this patch, that would
be fired after the already-existing BEFORE entry, gets its hands on
the NEW tuple values. I think that this makes the test more confusing
than it should, and that could cause unnecessary pain to understand
what's going on here for a future reader. Anyway, what's actually
the coverage we gain with this extra trigger in 003? The tests of
HEAD make already sure that if a trigger fires or not, so that seems
sufficient in itself. I guess that we could replace the existing
BEFORE trigger with something like what's proposed in this set to
track precisely which and when an operation happens on a relation with
a NEW and/or OLD set of tuples saved into this extra table, but the
interest looks limited for single relations.

On the other hand, the tests for partitions have much more value IMO,
but looking closely I think that we can do better:
- Create triggers on more relations of the partition tree,
particularly to also check when a trigger does not fire.
- Use a more generic name for tab1_2_op_log and its function
log_tab1_2_op(), say subs{1,2}_log_trigger_activity.
- Create some extra BEFORE triggers perhaps?

By the way, I had an idea of trick we could use to check if relations
do not leak: we could scan the logs for this pattern patterns,
similarly to what issues_sql_like() or connect_{fails,ok}() do
already, but that would mean increasing the log level and we don't do
that to ease the load of the nodes.
--
Michael