-
Notifications
You must be signed in to change notification settings - Fork 57
Refactor read-only iceberg table errors #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| if (PgLakeCopyValidityCheckHook) | ||
| PgLakeCopyValidityCheckHook(relationId); | ||
|
|
||
| ErrorIfReadOnlyIcebergTable(relationId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already do not push down such cases at IsCopyFromPushdownable.
| * deletion. | ||
| */ | ||
| if (!IsReadOnlyIcebergTable(relationId)) | ||
| if (IsWritableIcebergTable(relationId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsWritableIcebergTable is correct one after refactor. if block, which puts iceberg files to deletion queue, should only apply to writable iceberg tables.
| * throws an error if it is. | ||
| */ | ||
| void | ||
| ErrorIfReadOnlyIcebergTable(Oid relationId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not very much convinced that we get rid of ErrorIfReadOnlyIcebergTable, and introduce the same function in different places.
Can you remind me what is the reason not adjusting this one? I think having a single error message is even cleaner. For example, Postgres give a single error in read-only mode, which is a cleaner signal in the long term, compared to multiple different errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We error only for read-only iceberg tables at alter hook. We error for both read-only iceberg and pg_lake tables at truncate.
Question: Shouldn't we also throw error for read-only pg_lake tables during alter commands? If so, we can create a more generic error function that can be reused for all modification cases.
e3858c7 to
0d01cfc
Compare
563b36c to
1e5f0b7
Compare
Signed-off-by: Aykut Bozkurt <[email protected]>
1e5f0b7 to
aa9de74
Compare
Remove unnecessary read only iceberg table errors.
postgresIsForeignRelUpdatableshould handle all dml errors.We should throw error only for alter and truncate commands.