Duplicate key error in deletion_queue after post-commit REST catalog failure

[sfc-gh-npuka]

postgres$ INSERT INTO a values(0);
WARNING:  HTTP request failed (HTTP 429)
INSERT 0 1

postgres$ INSERT INTO a values(1);
ERROR:  duplicate key value violates unique constraint "deletion_queue_pkey"
DETAIL:  Key (path)=(s3://....metadata.json) already exists.
CONTEXT:  SQL statement "insert into lake_engine.deletion_queue
                         (path, table_name, orphaned_at, is_prefix) values ($1,$2,$3,$4)"

[sfc-gh-npuka]

Claude suggestion:

https://github.com/Snowflake-Labs/pg_lake/blob/main/pg_lake_engine/src/cleanup/deletion_queue.c#L362-L365
The INSERT at line 362-365 does a plain INSERT INTO with no conflict handling. When the same metadata path gets queued twice (e.g., due to a Polaris 429 where the transaction partially succeeded), it hits the primary key constraint.
The fix is to use ON CONFLICT DO NOTHING (or ON CONFLICT DO UPDATE if you want to refresh the timestamp):
deletion_queue.c

char	   *query =
"insert into " DELETION_QUEUE_TABLE " "
"(path, table_name, orphaned_at, is_prefix) "
"values ($1,$2,$3,$4) "
+ "ON CONFLICT (path) DO ...";

The same path being queued for deletion twice is harmless: it just means "delete this file eventually." Using ON CONFLICT (path) DO NOTHING silently skips the duplicate instead of crashing. The file will still get cleaned up from the first entry.

[sfc-gh-okalaci]

• "ON CONFLICT (path) DO ...";

I think that could easily hide real-bugs, issues as shown here.

We definitely considered doing that while developing the deletion queue, but that sounds wrong to me. Instead of silently allow this happening, I'd prefer seeing an error.

INSERT INTO a values(0);
WARNING: HTTP request failed (HTTP 429)
INSERT 0 1

This is a fundamental problem of REST catalog integration (see #94, first item in the list), where we need to commit both on Postgres and REST catalog.

And we currently prefer to commit on Postgres first, but then if commit on REST catalog fails -- such as your example --, we can only throw a warning, not an ERROR, because we are already at post-commit in Postgres, and throwing error would crash the server. So, we cannot undo whatever the transaction did to the database.

So, our goal is to do auto-recovery, and re-send the missing commit to REST catalog next time another commit happens on the database. And, apparently, as part of that auto-recovery, we should consider the deletion queue records. We can only do ON CONFLICT for that very special code-path, where are trying to recover from a failure.

[sfc-gh-okalaci]

WARNING: HTTP request failed (HTTP 429)

@sfc-gh-abozkurt why wouldn't ShouldRetryRequestToRestCatalog kick-in? Can you maybe debug a bit?

[sfc-gh-dachristensen]

Naïve question: isn't this exactly the sort of operation that 2PC was designed to facilitate? Is the issue with that slowness, not correctness?

[sfc-gh-okalaci]

Naïve question: isn't this exactly the sort of operation that 2PC was designed to facilitate? Is the issue with that slowness, not correctness?

Sure, but one server is Postgres the other is REST/polaris server. So we cannot directly use Postgres' 2PC mechanism (e.g., we cannot prepare on the REST catalog). We might still do some bookkeeping with REST catalog + 2PC, but see below, auto-recovery sounds easier?

At the core of the problem is that we want to commit on Postgres first, we want Postgres to be the authoritative source for the state. So, we defer the REST catalog operation to post-commit hook.

We have few ideas to handle the errors gracefully. First, we plan to do auto-recovery such that the next commit on Postgres on the same table creates a REST request that gets the REST catalog to the same state as Postgres. So, then the next successful commit is sort of error recovery on the REST catalog, that's why we refer that approach as auto-recovery. Another approach could be, as you alluded to, we can use some 2PC-like mechanism such that during the transaction we record what we send to REST catalog, and then some bg worker -- or the next transaction on the user backend -- might ensure if our changes are committed on REST.

The other option could a larger change, such that REST becomes the authoritative source for the state, such as #172, which requires some more thought. But this also allows other writers (e.g., snowflake) to write to the Iceberg table, and pg_lake can catch-up with the changes. So, more benefits.

[sfc-gh-mslot]

Perhaps inserting the metadata JSON into deletion queue is premature when the REST catalog has not committed yet? Wondering whether we can do that later, or confirm it later.