Re: SQL:2011 application time

On Tue, Sep 17, 2024 at 4:45 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> I have committed these, as explained here.
>
> I look forward to an updated patch set from you to review the "FOR
> PORTION OF" patches next.

Here are updates to the remaining patches.

I made big changes to FOR PORTION OF:

- Inserting the leftovers now uses SPI. Before I tried to use just
ExecInsert with a patched-up mtstate and other things, but it felt
brittle and had some problems. FOR EACH STATEMENT insert triggers were
not firing, and the transition table for the ROW triggers contained
both inserted tuples. According to the spec, we should treat the two
leftover inserts as separate statements. For example on DELETE,
15.7.8.c.ii (for the leading leftover) and 8.d.ii (for the trailing
leftover) both say:

> The following <insert statement> is effectively executed . . .

So each side gets a *separate* insert statement.

DB2 also fires statement triggers (one for each insert) and gives them
separate transition tables. Mariadb doesn't have statement triggers,
but you can tell they also treat each insert as a separate statement,
because row triggers fire like this: BEFORE INSERT, AFTER INSERT,
BEFORE INSERT, AFTER INSERT. One statement for both inserts would be
BEFORE BEFORE AFTER AFTER.

Then there were other things that worked but at the cost of more code,
like partitioned tables. Using SPI seems much more robust. To help
performance I am caching plans much as we do in ri_triggers.c.

- I cleaned up how I was handling some shift/reduce conflicts. The
problem was how INTERVALs accept a TO qualifier to keep details above
a given precision. For example INTERVAL '1:05:03' HOUR TO MINUTE still
drops the 3 seconds but not the 5 minutes. So this was ambiguous: FOR
PORTION OF valid_at FROM t1 + INTERVAL '1:05:03' HOUR TO t2.
Previously I was solving this with extra %nonassoc declarations, with
a guilty conscience, but now I'm using %prec and adding just TO to
IDENT's %nonassoc list. This is much more in line with the comment in
gram.y's precedence section.

- FOR PORTION OF a FROM b TO c now appears before the [[AS]
table_alias], which is where the spec says it should be. (Previously I
had it after the table alias.) This led to 30 more shift/reduce
conflicts, but solving them was pretty easy once I realized SELECT
with *column* aliases must have all the same problems, and I noticed
our bare_label_keyword list. So I'm doing something similar: if there
is no FOR PORTION OF, all the same grammar rules are used. If you have
FOR PORTION OF, then we allow [AS table_alias] always and just
[table_alias] in most cases. This did require a couple more %precs and
adding USING to the IDENT precedence level.

I haven't yet dealt with jian he's latest patch feedback, but I
thought this was significant enough progress to be worth sharing.

I also did some experiments with logical replication, especially
around REPLICA IDENTITY. For a temporal table, you have a primary key
but (1) it is not btree (2) it doesn't use equality for the WITHOUT
OVERLAPS part. So I would not be surprised to find problems there.
OTOH in principle it should be okay. Remember that a temporal primary
key is *strictly more restrictive* than a PK (at least now that we've
forbidden empty ranges). If you have all the key parts, that *does*
give you a unique record (whether you use equals or overlaps for the
last part, actually).

Logical *decoding* works fine. For updates/deletes, we report all the
key parts of the old record. For instance suppose I do this:

create extension btree_gist;
create table t (id integer, valid_at daterange, primary key (id,
valid_at without overlaps));
insert into t values (1, '[2000-01-01,2020-01-01)');
update t set id = 2;
update t for portion of valid_at from '2001-01-01' to '2002-01-01' set id = 3;

Then `pg_recvlogical` shows me this:

...
BEGIN 21535
table public.t: INSERT: id[integer]:1
valid_at[daterange]:'[2000-01-01,2020-01-01)'
COMMIT 21535
BEGIN 21536
table public.t: UPDATE: old-key: id[integer]:1
valid_at[daterange]:'[2000-01-01,2020-01-01)' new-tuple: id[integer]:2
valid_at[daterange]:'[2000-01-01,2020-01-01)'
COMMIT 21536
BEGIN 21537
table public.t: UPDATE: old-key: id[integer]:2
valid_at[daterange]:'[2000-01-01,2020-01-01)' new-tuple: id[integer]:3
valid_at[daterange]:'[2001-01-01,2002-01-01)'
table public.t: INSERT: id[integer]:2
valid_at[daterange]:'[2000-01-01,2001-01-01)'
table public.t: INSERT: id[integer]:2
valid_at[daterange]:'[2002-01-01,2020-01-01)'
COMMIT 21537

I think that is exactly what we want. Even FOR PORTION OF
updates/deletes replicate nicely: we send the update/delete and then
we send the inserts for leftovers.

On the `subscription` side, we also get what we want *if* we use
REPLICA IDENTITY FULL.

But if we use `REPLICA IDENTITY DEFAULT` based on a WITHOUT OVERLAPS
primary key, the subscriber can't handle the GiST index. We get an
error like this:

2024-10-12 22:47:09.882 CDT [69308] ERROR: missing operator 0(23,23)
in opfamily 16503
2024-10-12 22:47:09.882 CDT [69308] CONTEXT: processing remote data
for replication origin "pg_17080" during message type "UPDATE" for
replication target relation "public.t" in transaction 21542, finished
at 0/11D9EEE0
2024-10-12 22:47:09.883 CDT [66619] LOG: background worker "logical
replication apply worker" (PID 69308) exited with exit code 1
2024-10-12 22:47:14.874 CDT [69420] LOG: logical replication apply
worker for subscription "s" has started

I will work on a fix for that, but I think for now we could also raise
an error on the publication side if you try to replicate
updates/deletes through a temporal PK, and then document that temporal
tables need to use `REPLICA IDENTITY FULL`.

I'll try to get to jian he's patch feedback soon as well.

And then there are a couple more bits of followup I'd like to do:

- Should we pass the FOR PORTION OF clause to FDWs? I think we should,
and it can be up to them whether they support it. But in the meantime
I think we can just say that FOR PORTION OF against FDWs is not
supported.

- We should allow the FOR PORTION OF FROM/TO bounds to be named
parameters. For instance you can't do this:

CREATE FUNCTION fpo_sql_update(target_from date, target_til date)
RETURNS VOID
AS $$
UPDATE for_portion_of_test2
FOR PORTION OF valid_at
FROM target_from TO target_til
SET name = concat(name, '*')
WHERE id = '[1,2)';
$$
LANGUAGE sql;

The problem is that ColumnRefs aren't allowed as bounds, and that's
how we parse the parameters here. PL/pgSQL functions act the same. A
workaround is to use FROM $1 TO $2, which *does* work. Or of course
format + EXECUTE. Given those alternatives, this seems like a
low-priority problem, but I'm still interested in fixing it.

Actually I've already had at least one person ask for genuine
ColumnRefs in FOR PORTION OF bounds. According to the spec those
aren't allowed, and we currently evaluate them in ExecInitModifyTable,
but in principle I don't see any reason we couldn't permit them.
Perhaps we could even still evaluate them upfront if we can see they
are constant.

Rebased to c0b74323dc.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com