Select the DeleteObjects secret from the objects, not the bucket by sfc-gh-mslot · Pull Request #655 · Snowflake-Labs/pg_lake

sfc-gh-mslot · 2026-09-23T22:01:34Z

Problem

A batched DeleteObjects request carries its keys in the body of a POST to
<bucket>/?delete, so the bucket URL is the only URL the request itself names.
The batch delete path used that URL for more than the request target: it read
the S3 auth parameters from it and built the signing handle from it.

That is not where the credentials for those objects necessarily live. DuckDB
selects a secret by longest matching URL prefix, so a secret scoped to
s3://bucket/prefix does not match the bucket URL. A caller holding a secret
for exactly the prefix it is deleting from gets its request signed with
whatever else happens to cover the bucket — a bucket-wide secret, or the
default one — and S3 refuses the delete even though a correct credential for
those objects was configured.

The credential-refresh retry has the same problem from the other side: on a
401/403 it looks the secret up by URL, so refreshing on the bucket URL can
replace correct prefix-scoped credentials with another scope's.

The single-file RemoveFile path already read its credentials from the object
being deleted, so only the batch path was affected.

Solution

Take the credentials from the objects rather than from the bucket, i.e. from
the first path of each batch, and group the paths so that is well defined:
one request per (bucket URL, resolved credentials) pair. One request carries
one signature, and a single bucket can hold several prefixes each with a secret
of its own, so grouping by bucket alone is not enough.

The region is resolved on that same first path, so paths carrying an explicit
s3_region or s3_endpoint still fall back to the single-file delete exactly
as before, as do non-S3 paths.

PostRequestForCredentialScope takes the URL a credential refresh looks the
secret up by separately from the URL the request goes to.
PostRequest passes the same URL for both, which is what every other caller
wants; only DeleteObjects needs them to differ.

Test plan

Two new pytests in pgduck_server/tests/pytests/test_s3.py, both against the
IAM-enforcing moto fixture, so credentials are actually checked:

test_batch_delete_signs_with_the_objects_own_secret — objects under a
prefix, a prefix-scoped secret whose user may delete them, and a bucket-wide
secret whose user may not. The multi-row pg_lake_remove_file statement must
succeed and the objects must be gone.
test_batch_delete_does_not_share_one_secret_across_scopes — two prefixes in
one bucket with a secret each, the second naming a key that is not a
principal. The batch for the first prefix goes through, the second is
refused, and its objects are still there: they were never signed with the
first prefix's credentials.

Both fail on the current code (the first with AccessDenied on all keys, the
second because all four objects get deleted with one credential) and pass with
this change. test_s3.py (19 passed) and test_caching.py (30 passed, 1
skipped) are green, and the duckdb_pglake release build is clean.

test_common/helpers/moto_iam_storage.py gains create_bucket_user: moto
authorizes a whole DeleteObjects request against the bucket ARN before it
checks the individual keys, so a user whose object rights stop at a prefix is
refused the request for that unrelated reason. A test about which credential
a request carries needs a user that is not refused for that other reason.

A batched DeleteObjects request carries its keys in the body of a POST to <bucket>/?delete, so the bucket URL is the only URL the request names. The batch path read its credentials and built its signing handle from that URL, which is not where the credentials for those objects necessarily live: DuckDB picks a secret by longest matching URL prefix, and a secret scoped to s3://bucket/prefix does not match the bucket URL. A caller holding a secret for exactly the prefix it is deleting from would have the request signed with whatever else covered the bucket, or with the default secret. Take the credentials from the first object of the batch instead, and group the paths accordingly: one request per (bucket, resolved credentials) pair, since one request also carries only one signature and a single bucket can hold several prefixes with a secret of their own. The region is resolved on that same first path, so an explicit region or endpoint on a path still falls back to the single-file delete as before. The credential-refresh retry on a 401/403 needs the same treatment: it looks the secret up by URL, so refreshing on the bucket URL could replace working prefix-scoped credentials with another scope's. PostRequestForCredentialScope takes the lookup URL separately from the request URL; PostRequest passes the same URL for both, as every other caller wants. The single-file delete path already read its credentials from the object and is unchanged. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

sfc-gh-dachristensen · 2026-09-23T22:37:28Z

-	map<string, vector<string>> pathsByBucket;
+	vector<S3DeleteBatch> batches;

 	for (const string &path : paths)


These are expected to be ordered by prefix so credential grouping naturally falls out from that?

the idea is that for each path we linearly search for a batch that matches based on s3 credentials used

(most of the time, just 1 batch)

The summary counted queue rows claimed, not objects removed. The in-progress drain flushes its batch with DeleteRemoteFileBatch(batch, NULL, NULL), which has no outcome list to record, so a remote delete that fails is reported as a WARNING and the path still lands on the removed list and still loses its catalog row. The summary then read as a completed cleanup on top of the warning that said otherwise. The per-file "deleting unused file" LOG already names every path the drain touches, and a failure reports itself next to it, so the absence of a warning after those lines is the completion signal. The counter stays: it is what the per-vacuum removal budget and VacuumStoppedWithFilesQueued are charged against. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

RemoveInProgressFiles drops the in_progress_files row for every path it claims, whatever remote storage made of the removal: the table has no retry column, so a claimed row is gone either way. Counting claimed rows therefore reported files that had not been removed, directly beneath the warning saying the removal failed. Split the two numbers. RemoveInProgressFiles now returns the paths remote storage confirmed gone in *removedPaths and, for callers that need it, every path it claimed in *claimedPaths. VacuumRemoveInProgressFiles reports removals and charges its per-vacuum budget claims -- charging removals would let a path that failed be re-claimed by the next iteration without advancing the budget, so the loop would stop terminating. flush_in_progress_queue returns confirmed removals for the same reason. Asking DeleteRemoteFileBatch for the removed paths also means a failed batch is retried a path at a time, so one unremovable path no longer hides the rest of its batch from the count. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

The autovacuum worker runs the object store catalog export in the same serial loop as the removal stages, so the export only gets a turn once a removal phase returns, and max_file_removals_per_vacuum is the only thing bounding how long that takes. At 100000 it bounded nothing useful: a healthy object store spends that budget in about a hundred batched requests, while a failing one retries a batch a path at a time and can spend it in a hundred thousand single-object requests -- hours per table in which no catalog is published. 10000 keeps the healthy case whole (ten full deletion batches per pass, and a pass that stops on the budget re-enters without a naptime, so drain throughput is unchanged) and cuts the worst case tenfold. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

…pass An in-progress row is deleted after the removal attempt whether or not the object went away, which has been true since the first commit: the bool from DeleteRemoteFile/DeleteRemotePrefix was never checked, and batching the catalog DELETE kept that. The row is the only record that the file exists, so a failed attempt left the object behind with nothing left to retry it. Only paths remote storage confirmed gone now lose their row, so the rest are tried again by a later cleanup cycle. Both cleanup walks now stop at the first path they could not remove, rather than working through the rest of the claimed rows and retrying every path of a failed batch one at a time. Whatever the object store is unhappy about is rarely specific to one key, so the remaining attempts mostly buy a request and a WARNING each, while the vacuum cycle -- and with it catalog publication -- waits behind them. The rows left unattempted keep their queue row, and in the deletion queue also their retry_count, so the next pass picks them up; the path that failed is held off by vacuum_file_remove_retry_interval there. Since a failed in-progress path now keeps its row, the per-vacuum budget is charged removals rather than claims, and a failed pass reports no remaining files so the loop does not re-claim the path it just failed on. There is no retry state on in_progress_files, so those attempts are spaced by how often cleanup runs rather than by a clock of their own. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

Keeping the row in in_progress_files is what stops a failed removal from being forgotten, but that table has nowhere to record an attempt: the path is claimed by every pass with nothing to space the attempts out, and because the walk stops at the first failure, a path that keeps failing holds up the rows behind it for as long as it lasts. Hand it to the deletion queue instead, which is the queue with retry state: a retry count, a retry interval between attempts, and a cap that eventually retires a path. The row carries no table, because a path is all the in-progress queue knows about it, so it is claimed by the dropped-table pass that the autovacuum worker runs ahead of the per-table ones. The attempt that just failed is recorded as the first one, so the next is held off for vacuum_file_remove_retry_interval rather than made again in the following pass, and a path already in the queue is left as it is rather than aborting the pass -- including its successful removals -- with a unique violation. Both queues are written in the same subtransaction as the DELETE, so a path cannot leave one without arriving in the other. Paths the pass never reached keep their in-progress rows and are claimed again by a later cycle. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

The requeue test left its unremovable path in the deletion queue when an assertion failed ahead of the cleanup, and a path that cannot be removed stops a drain before it reaches the paths behind it. VACUUM FULL skips the retry interval, so every later test in the same database that waited for its own files to go failed too. Do the cleanup in a finally, and compare the row against the list of values run_query returns. RemoveInProgressFiles appends to the list it is given, and the list is allocated in the transaction each iteration of the loop commits. Carrying it into the next iteration appends to and reads memory that is gone, and counts the same paths again. Start each iteration empty, and report nothing for a pass whose removals went back with its subtransaction. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

removedFiles was assigned through the pointer in PG_TRY, assigned again in PG_CATCH and read after PG_END_TRY, without volatile, so a longjmp could leave the compiler reading a stale register. Adding the qualifier does not work directly: &removedFiles then no longer matches RemoveInProgressFiles' List ** parameter. Count the paths where they are produced instead. The list no longer crosses the sigsetjmp boundary and needs no qualifier, the counter that does cross is volatile, and a pass that threw reports nothing because it never reaches the increment. This is the shape VacuumRemoveDeletionQueueRecords already uses. Signed-off-by: Marco Slot <marco.slot@snowflake.com>

… (#658) * Select the DeleteObjects secret from the objects, not the bucket A batched DeleteObjects request carries its keys in the body of a POST to <bucket>/?delete, so the bucket URL is the only URL the request names. The batch path read its credentials and built its signing handle from that URL, which is not where the credentials for those objects necessarily live: DuckDB picks a secret by longest matching URL prefix, and a secret scoped to s3://bucket/prefix does not match the bucket URL. A caller holding a secret for exactly the prefix it is deleting from would have the request signed with whatever else covered the bucket, or with the default secret. Take the credentials from the first object of the batch instead, and group the paths accordingly: one request per (bucket, resolved credentials) pair, since one request also carries only one signature and a single bucket can hold several prefixes with a secret of their own. The region is resolved on that same first path, so an explicit region or endpoint on a path still falls back to the single-file delete as before. The credential-refresh retry on a 401/403 needs the same treatment: it looks the secret up by URL, so refreshing on the bucket URL could replace working prefix-scoped credentials with another scope's. PostRequestForCredentialScope takes the lookup URL separately from the request URL; PostRequest passes the same URL for both, as every other caller wants. The single-file delete path already read its credentials from the object and is unchanged. * Drop the orphaned-file cleanup summary log The summary counted queue rows claimed, not objects removed. The in-progress drain flushes its batch with DeleteRemoteFileBatch(batch, NULL, NULL), which has no outcome list to record, so a remote delete that fails is reported as a WARNING and the path still lands on the removed list and still loses its catalog row. The summary then read as a completed cleanup on top of the warning that said otherwise. The per-file "deleting unused file" LOG already names every path the drain touches, and a failure reports itself next to it, so the absence of a warning after those lines is the completion signal. The counter stays: it is what the per-vacuum removal budget and VacuumStoppedWithFilesQueued are charged against. * Report the files vacuum removed, not the rows it claimed RemoveInProgressFiles drops the in_progress_files row for every path it claims, whatever remote storage made of the removal: the table has no retry column, so a claimed row is gone either way. Counting claimed rows therefore reported files that had not been removed, directly beneath the warning saying the removal failed. Split the two numbers. RemoveInProgressFiles now returns the paths remote storage confirmed gone in *removedPaths and, for callers that need it, every path it claimed in *claimedPaths. VacuumRemoveInProgressFiles reports removals and charges its per-vacuum budget claims -- charging removals would let a path that failed be re-claimed by the next iteration without advancing the budget, so the loop would stop terminating. flush_in_progress_queue returns confirmed removals for the same reason. Asking DeleteRemoteFileBatch for the removed paths also means a failed batch is retried a path at a time, so one unremovable path no longer hides the rest of its batch from the count. * Lower max_file_removals_per_vacuum to 10000 The autovacuum worker runs the object store catalog export in the same serial loop as the removal stages, so the export only gets a turn once a removal phase returns, and max_file_removals_per_vacuum is the only thing bounding how long that takes. At 100000 it bounded nothing useful: a healthy object store spends that budget in about a hundred batched requests, while a failing one retries a batch a path at a time and can spend it in a hundred thousand single-object requests -- hours per table in which no catalog is published. 10000 keeps the healthy case whole (ten full deletion batches per pass, and a pass that stops on the budget re-enters without a naptime, so drain throughput is unchanged) and cuts the worst case tenfold. * Keep in-progress files queued when their removal fails, and stop the pass An in-progress row is deleted after the removal attempt whether or not the object went away, which has been true since the first commit: the bool from DeleteRemoteFile/DeleteRemotePrefix was never checked, and batching the catalog DELETE kept that. The row is the only record that the file exists, so a failed attempt left the object behind with nothing left to retry it. Only paths remote storage confirmed gone now lose their row, so the rest are tried again by a later cleanup cycle. Both cleanup walks now stop at the first path they could not remove, rather than working through the rest of the claimed rows and retrying every path of a failed batch one at a time. Whatever the object store is unhappy about is rarely specific to one key, so the remaining attempts mostly buy a request and a WARNING each, while the vacuum cycle -- and with it catalog publication -- waits behind them. The rows left unattempted keep their queue row, and in the deletion queue also their retry_count, so the next pass picks them up; the path that failed is held off by vacuum_file_remove_retry_interval there. Since a failed in-progress path now keeps its row, the per-vacuum budget is charged removals rather than claims, and a failed pass reports no remaining files so the loop does not re-claim the path it just failed on. There is no retry state on in_progress_files, so those attempts are spaced by how often cleanup runs rather than by a clock of their own. * Requeue a failed in-progress removal instead of retrying it in place Keeping the row in in_progress_files is what stops a failed removal from being forgotten, but that table has nowhere to record an attempt: the path is claimed by every pass with nothing to space the attempts out, and because the walk stops at the first failure, a path that keeps failing holds up the rows behind it for as long as it lasts. Hand it to the deletion queue instead, which is the queue with retry state: a retry count, a retry interval between attempts, and a cap that eventually retires a path. The row carries no table, because a path is all the in-progress queue knows about it, so it is claimed by the dropped-table pass that the autovacuum worker runs ahead of the per-table ones. The attempt that just failed is recorded as the first one, so the next is held off for vacuum_file_remove_retry_interval rather than made again in the following pass, and a path already in the queue is left as it is rather than aborting the pass -- including its successful removals -- with a unique violation. Both queues are written in the same subtransaction as the DELETE, so a path cannot leave one without arriving in the other. Paths the pass never reached keep their in-progress rows and are claimed again by a later cycle. * Clean up the requeued path, and scope removedFiles to one vacuum pass The requeue test left its unremovable path in the deletion queue when an assertion failed ahead of the cleanup, and a path that cannot be removed stops a drain before it reaches the paths behind it. VACUUM FULL skips the retry interval, so every later test in the same database that waited for its own files to go failed too. Do the cleanup in a finally, and compare the row against the list of values run_query returns. RemoveInProgressFiles appends to the list it is given, and the list is allocated in the transaction each iteration of the loop commits. Carrying it into the next iteration appends to and reads memory that is gone, and counts the same paths again. Start each iteration empty, and report nothing for a pass whose removals went back with its subtransaction. * Keep the in-progress removal count inside the subtransaction removedFiles was assigned through the pointer in PG_TRY, assigned again in PG_CATCH and read after PG_END_TRY, without volatile, so a longjmp could leave the compiler reading a stale register. Adding the qualifier does not work directly: &removedFiles then no longer matches RemoveInProgressFiles' List ** parameter. Count the paths where they are produced instead. The list no longer crosses the sigsetjmp boundary and needs no qualifier, the counter that does cross is volatile, and a pass that threw reports nothing because it never reaches the increment. This is the shape VacuumRemoveDeletionQueueRecords already uses. --------- (cherry picked from commit a243c41) Signed-off-by: Marco Slot <marco.slot@snowflake.com>

sfc-gh-mslot requested review from sfc-gh-abozkurt, sfc-gh-dachristensen and sfc-gh-okalaci as code owners September 23, 2026 22:01

sfc-gh-dachristensen approved these changes Sep 23, 2026

View reviewed changes

sfc-gh-mslot requested a review from sfc-gh-amzheng as a code owner September 24, 2026 07:50

sfc-gh-mslot force-pushed the marcoslot/delete-objects-secret-scope branch from ea5ce9f to 11481ee Compare September 24, 2026 08:26

sfc-gh-mslot force-pushed the marcoslot/delete-objects-secret-scope branch from a789b51 to 6f4c6f2 Compare September 24, 2026 10:58

sfc-gh-mslot added 2 commits September 24, 2026 11:21

sfc-gh-mslot added the backport label Sep 24, 2026

sfc-gh-mslot added 2 commits September 24, 2026 11:59

sfc-gh-mslot merged commit a243c41 into main Sep 24, 2026
173 of 174 checks passed

sfc-gh-mslot deleted the marcoslot/delete-objects-secret-scope branch September 24, 2026 13:26

Navigation Menu

Select the DeleteObjects secret from the objects, not the bucket - #655

Select the DeleteObjects secret from the objects, not the bucket#655
sfc-gh-mslot merged 8 commits into
mainfrom
marcoslot/delete-objects-secret-scope

sfc-gh-mslot commented Sep 23, 2026

Uh oh!

sfc-gh-dachristensen Sep 23, 2026

Uh oh!

sfc-gh-mslot Sep 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sfc-gh-mslot commented Sep 23, 2026

Problem

Solution

Test plan

Uh oh!

sfc-gh-dachristensen Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

sfc-gh-mslot Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sfc-gh-mslot Sep 24, 2026 •

edited

Loading