Thread: [9.3] Automatically updatable views vs writable foreign tables
Hi, I've just started 9.3 beta testing and I noticed that a "simple" view defined on top of a writable foreign table is not automatically updatable. Given that these are both new-to-9.3 features, I think it would be a shame if they don't work together. It's basically a 1-line patch to make such views automatically updatable, plus a small extra code block in relation_is_updatable() to reflect the change in the information_schema views. The attached patch does that and adds a couple of extra regression tests. The tests, however, reveal a separate issue with writable foreign tables --- the information_schema views haven't been updated to reflect the fact that foreign tables may now be updatable. Specifically, for foreign tables information_schema.tables.is_insertable_into and information_schema.columns.is_updatable always say 'NO' even if the foreign table is writable. Fixing that would require new C functions along the same lines as pg_view_is_insertable/updatable(), or those functions could just be renamed and repurposed to do the check for all relation kinds, except those known to be always/never updatable. Regards, Dean
Attachment
Dean Rasheed <dean.a.rasheed@gmail.com> writes: > I've just started 9.3 beta testing and I noticed that a "simple" view > defined on top of a writable foreign table is not automatically > updatable. > Given that these are both new-to-9.3 features, I think it would be a > shame if they don't work together. It's basically a 1-line patch to > make such views automatically updatable, plus a small extra code block > in relation_is_updatable() to reflect the change in the > information_schema views. Meh. This is assuming that an FDW that defines, say, ExecForeignDelete is thereby promising that *all* tables it supports are deletable. That is not required by the current FDW API spec. If we want to do something about this, I'd be a bit inclined to say that we should add a new FDW callback function to let the FDW say whether a particular rel is updatable or not. I think it would be a good idea to get that done for 9.3, since all this support is new in 9.3, and it's not too late to adjust the API now. If we wait, there will be compatibility headaches. > Specifically, for foreign tables > information_schema.tables.is_insertable_into and > information_schema.columns.is_updatable always say 'NO' even if the > foreign table is writable. Fixing that would require new C functions > along the same lines as pg_view_is_insertable/updatable(), or those > functions could just be renamed and repurposed to do the check for all > relation kinds, except those known to be always/never updatable. I'd vote to rename/extend them to be pg_relation_is_updatable I think. regards, tom lane
On 05/16/2013 05:16 PM, Tom Lane wrote: > Dean Rasheed <dean.a.rasheed@gmail.com> writes: >> I've just started 9.3 beta testing and I noticed that a "simple" view >> defined on top of a writable foreign table is not automatically >> updatable. >> Given that these are both new-to-9.3 features, I think it would be a >> shame if they don't work together. It's basically a 1-line patch to >> make such views automatically updatable, plus a small extra code block >> in relation_is_updatable() to reflect the change in the >> information_schema views. > Meh. This is assuming that an FDW that defines, say, ExecForeignDelete > is thereby promising that *all* tables it supports are deletable. That > is not required by the current FDW API spec. > > If we want to do something about this, I'd be a bit inclined to say that > we should add a new FDW callback function to let the FDW say whether > a particular rel is updatable or not. > > I think it would be a good idea to get that done for 9.3, since all this > support is new in 9.3, and it's not too late to adjust the API now. > If we wait, there will be compatibility headaches. +1 cheers andrew
On 16 May 2013 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > This is assuming that an FDW that defines, say, ExecForeignDelete > is thereby promising that *all* tables it supports are deletable. That > is not required by the current FDW API spec. > Ah OK, I didn't appreciate that distinction. > If we want to do something about this, I'd be a bit inclined to say that > we should add a new FDW callback function to let the FDW say whether > a particular rel is updatable or not. > > I think it would be a good idea to get that done for 9.3, since all this > support is new in 9.3, and it's not too late to adjust the API now. > If we wait, there will be compatibility headaches. > +1. That seems like something that should be part of the API, even if we didn't have an immediate use for it. Regards, Dean
On 16 May 2013 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Specifically, for foreign tables >> information_schema.tables.is_insertable_into and >> information_schema.columns.is_updatable always say 'NO' even if the >> foreign table is writable. Fixing that would require new C functions >> along the same lines as pg_view_is_insertable/updatable(), or those >> functions could just be renamed and repurposed to do the check for all >> relation kinds, except those known to be always/never updatable. > > I'd vote to rename/extend them to be pg_relation_is_updatable I think. > I remember now just how ugly this code is. The SQL standard has separate concepts of trigger-insertable, trigger-updatable, trigger-deletable, insertable and updatable but not deletable for some reason. So we define updatable as supporting both UPDATE and DELETE without triggers. I think what we have implemented is technically correct with regards to the spec in this area, but it is not particularly useful as far as telling whether a relation will actually support a particular query in practice (for example a simple view on top of a trigger-updatable view is neither updatable nor trigger-updatable). One place where I think we have diverged from the spec, however, is in information_schema.columns.updatable. This should be returning 'YES' if the individual column is updatable, and I see no reason for that the require the relation to support DELETE, which is what we currently do (and always have done). To implement the information_schema properly per-spec, I think we need 3 functions: pg_relation_is_insertable(), pg_relation_is_updatable() and pg_column_is_updatable(), with the latter just checking UPDATE events. It's probably a good idea to add these functions now, since I hope in the future to support more of the SQL spec regarding automatically updatable views, which will involve views for which only a subset of their columns are updatable. The attached patch does that, and tightens up relation_is_updatable() to support all relation kinds, but it still assumes that if a FDW defines, say, ExecForeignUpdate, then all its foreign tables are updatable. That could be improved upon by defining new FDW API callbacks that select from the remote information_schema, but I'm now starting to doubt whether its really worth the trouble, given its bizzare definition. Regards, Dean
Attachment
looking at this patch some more ... Dean Rasheed <dean.a.rasheed@gmail.com> writes: > One place where I think we have diverged from the spec, however, is in > information_schema.columns.updatable. This should be returning 'YES' > if the individual column is updatable, and I see no reason for that > the require the relation to support DELETE, which is what we currently > do (and always have done). I'm not convinced about this change. The spec's notion of updatability requires both UPDATE and DELETE to be allowed; that's why they don't have a separate is_deletable attribute. And they don't have any such thing as a column whose updatability doesn't require updatability of the underlying table. So I think the previous behavior was correct and should be maintained: although Postgres does permit decoupling deletability from updatability, only tables/columns for which both operations are possible should be marked is_updatable in the information_schema. Otherwise, an application relying on the assumption that "is_updatable" means it can DELETE will be broken. I can see however that varying opinions on this are possible. Although I'd removed the separate pg_column_is_updatable() function from your patch with the intent of using pg_relation_is_updatable() directly, I'm now thinking about putting back the former, so that this decision is taken in C code where we can change it without an initdb. regards, tom lane
On 12 June 2013 18:35, Tom Lane <tgl@sss.pgh.pa.us> wrote: > looking at this patch some more ... > > Dean Rasheed <dean.a.rasheed@gmail.com> writes: >> One place where I think we have diverged from the spec, however, is in >> information_schema.columns.updatable. This should be returning 'YES' >> if the individual column is updatable, and I see no reason for that >> the require the relation to support DELETE, which is what we currently >> do (and always have done). > > I'm not convinced about this change. The spec's notion of updatability > requires both UPDATE and DELETE to be allowed; that's why they don't > have a separate is_deletable attribute. And they don't have any such > thing as a column whose updatability doesn't require updatability of the > underlying table. So I think the previous behavior was correct and > should be maintained: although Postgres does permit decoupling > deletability from updatability, only tables/columns for which both > operations are possible should be marked is_updatable in the > information_schema. Otherwise, an application relying on the assumption > that "is_updatable" means it can DELETE will be broken. > > I can see however that varying opinions on this are possible. Although > I'd removed the separate pg_column_is_updatable() function from your > patch with the intent of using pg_relation_is_updatable() directly, > I'm now thinking about putting back the former, so that this decision > is taken in C code where we can change it without an initdb. > The more I read the spec, the less sense it seems to make, and each time I read it, I seem to reach a different conclusion. On my latest reading, I've almost convinced myself that "updatable" is meant to imply support for all 3 operations (INSERT, UPDATE and DELETE), at least in the absence of transient tables. The descriptions of all 3 seem to require the table to be updatable. INSERT requires the table to be insertable-into, updatable and all its columns to be updatable, but the requirement for insertable-into is only to rule out transient tables. So if you don't have transient tables, which aren't insertable-into, then all 3 operations are possible if and only if the table is updatable. That interpretation could be used to simplify the API, but no doubt when I re-read the spec tomorrow, I'll reach a different conclusion. Regards, Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes: > The more I read the spec, the less sense it seems to make, and each > time I read it, I seem to reach a different conclusion. > On my latest reading, I've almost convinced myself that "updatable" is > meant to imply support for all 3 operations (INSERT, UPDATE and > DELETE), at least in the absence of transient tables. The descriptions > of all 3 seem to require the table to be updatable. Still, they do admit the possibility of insertable_into being different from is_updatable. So I'm pretty happy with what we've got, at least on the relation level. Columns seem a bit more debatable; though I continue to think that an is_updatable column in a not-is_updatable table isn't contemplated by the spec. regards, tom lane
On 13 June 2013 01:11, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dean Rasheed <dean.a.rasheed@gmail.com> writes: >> The more I read the spec, the less sense it seems to make, and each >> time I read it, I seem to reach a different conclusion. > >> On my latest reading, I've almost convinced myself that "updatable" is >> meant to imply support for all 3 operations (INSERT, UPDATE and >> DELETE), at least in the absence of transient tables. The descriptions >> of all 3 seem to require the table to be updatable. > > Still, they do admit the possibility of insertable_into being different > from is_updatable. So I'm pretty happy with what we've got, at least > on the relation level. Columns seem a bit more debatable; though I > continue to think that an is_updatable column in a not-is_updatable > table isn't contemplated by the spec. > Of course if we didn't have rules, this wouldn't be as issue, because then a view that handled one update operation would handle them all. The spec doesn't need to worry about that, so it can define the updatability of a view as a singular concept based on the view's definition; and insertable_into in terms of the properties of the base table. In that context, the possibility of an is_updatable column in a not-is_updatable table doesn't need to be considered. I don't think that any more reading of the spec is going to help, because it's simply not as issue that they had to worry about. If the spec did consider rules, it would probably define rule_insertable, etc., in the same way as triggers. So our problem is in trying to shoe-horn rule-updatability into the spec's idea of updatability, and it doesn't really fit. The more technically correct answer might be to say that rule-updatable doesn't count as updatable any more than trigger-updatable does, but that wouldn't be very useful in practice because there are no columns in the information schema to check for rule-updatability. So really, I think we're trying to come up with the most practically useful definition, and in that context I think we've probably done the right thing at the relation-level, but I still think that a column could be marked as is_updatable, even if the table didn't support DELETEs. That said, I think that this is of such limited interest to anyone that I'm inclined to simply keep the status quo. Regards, Dean