On Thu, Apr 22, 2021 at 1:45 PM Michael Paquier <michael@paquier.xyz> wrote:
> 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.
Thanks for that.
> 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?
Not much maybe. I am fine with dropping the changes made to 003 if
they are confusing, which I agree they can be.
> 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.
It seems you're suggesting to adopt 003's trigger firing tests for
partitions in 013, but would we gain much by that?
> - Use a more generic name for tab1_2_op_log and its function
> log_tab1_2_op(), say subs{1,2}_log_trigger_activity.
Sure, done.
> - Create some extra BEFORE triggers perhaps?
Again, that seems separate from what we're trying to do here. AIUI,
our aim here is to expand coverage for after triggers, and not really
that of the trigger functionality proper, because nothing seems broken
about it, but that of the trigger target relation opening/closing.
ISTM that's what you're talking about below...
> 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,
It would be interesting to be able to do something like that, but....
> 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.
...sorry, I am not very familiar with our Perl testing infra. Is
there some script that already does something like this? For example,
checking in the logs generated by a "node" that no instance of a
certain WARNING is logged?
Meanwhile, attached is the updated version containing some of the
changes mentioned above.
--
Amit Langote
EDB: http://www.enterprisedb.com
