On 1/24/24 08:32, Peter Eisentraut wrote:
> On 18.01.24 04:59, Paul Jungwirth wrote:
>> Here are new patches consolidating feedback from several emails.
>
> I have committed 0001 and 0002 (the primary key support).
Thanks Peter! I noticed the comment on gist_stratnum_btree was out-of-date, so here is a tiny patch
correcting it.
Also the remaining patches with some updates:
I fixed the dependency issues with PERIODs and their (hidden) GENERATED range columns. This has been
causing test failures and bugging me since I reordered the patches at PgCon, so I'm glad to finally
clean it up. The PERIOD should have an INTERNAL dependency on the range column, but then when you
dropped the table the dependency code thought the whole table was part of the INTERNAL dependency,
so the drop would fail. The PERIOD patch here fixes the dependency logic. (I guess this is the first
time a column has been an internal dependency of something.)
I also fixed an error message when you try to change the type of a start/end column used by a
PERIOD. Previously the error message would complain about the GENERATED column, not the PERIOD,
which seems confusing. In fact it was non-deterministic, depending on which pg_depend record the
index returned first.
On 12/6/23 05:22, jian he wrote:
> tring to the following TODO:
> // TODO: Need to save context->mtstate->mt_transition_capture? (See
> comment on ExecInsert)
>
> but failed.
> I also attached the trial, and also added the related test.
>
> You can also use the test to check portion update with insert trigger
> with "referencing old table as old_table new table as new_table"
> situation.
Thank you for the very helpful test case here. I fixed the issue of not passing along the transition
table. But there is still more work to do here I think:
- The AFTER INSERT FOR EACH ROW triggers have *both* leftover rows in the NEW table. Now the docs do
say that for AFTER triggers, a named transition table can see all the changes from the *statement*
(although that seems pretty weird to me), but the inserts are two *separate* statements. I think the
SQL:2011 standard is fairly clear about that. So each time the trigger fires we should still get
just one row in the transition table.
- The AFTER INSERT FOR EACH STATEMENT triggers never fire. That happens outside ExecInsert (in
ExecModifyTable). In fact there is a bunch of stuff in ExecModifyTable that maybe we need to do when
we insert leftovers. Do we even need a separate exec node, perhaps wrapping ExecModifyTable? I'm not
sure that would give us the correct trigger ordering for the triggers on the implicit insert
statement(s) vs the explicit update/delete statement, so maybe it does all need to be part of the
single node. But still I think we need to be more careful about memory, especially the per-tuple
context.
I'll keep working on that, but at least in this round of patches the transition tables aren't
missing completely.
My plan is still to replace the 'p' amoppurpose operators with just support functions. I want to do
that next, although as Peter requested I'll also start focusing more narrowly on the foreign key
patches.
Rebased to 46a0cd4cef.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
