Re: [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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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(dot)a(dot)rasheed(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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

looking at this patch some more ...

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> looking at this patch some more ...
>
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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(dot)a(dot)rasheed(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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