Horton

Thread

Commits

  1. Fix unexpected error messages for various flavors of ALTER TABLE

  2. Revert per-index collation version tracking feature.

  3. Restructure ALTER TABLE execution to fix assorted bugs.

  1. ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-16T09:14:15Z

    Hello,

If I invoked a wrong ALTER TABLE command like this, I would see an
unexpected error.

=# ALTER TABLE <foreign table> ATTACH PARTITION ....
ERROR:  "ft1" is of the wrong type

The cause is that ATWrongRelkidError doesn't handle ATT_TABLE |
ATT_ATT_PARTITIONED_INDEX.

After checking all callers of ATSimplePermissions, I found that;

The two below are no longer used.

  ATT_TABLE | ATT_VIEW
  ATT_TABLE | ATT_MATVIEW | ATT_INDEX

The four below are not handled.

  ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
  ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
  ATT_TABLE | ATT_PARTITIONED_INDEX:
  ATT_INDEX:

The attached is just fixing that.  I tried to make it generic but
didn't find a clean and translatable way.

Also I found that only three cases in the function are excecised by
make check.

ATT_TABLE                     : foreign_data, indexing checks 
ATT_TABLE | ATT_FOREIGN_TABLE : alter_table
ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table

I'm not sure it's worth the trouble so the attached doesn't do
anything for that.


Versions back to PG11 have similar but different mistakes.

PG11, 12:
 the two below are not used
   ATT_TABLE | ATT_VIEW
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX

 the two below are not handled
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
   ATT_TABLE | ATT_PARTITIONED_INDEX

PG13:
 the two below are not used
   ATT_TABLE | ATT_VIEW
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX

 the three below are not handled
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
   ATT_TABLE | ATT_PARTITIONED_INDEX

PG10:
  ATT_TABLE | ATT_VIEW is not used
  (all values are handled)

So the attached are the patches for PG11, 12, 13 and master.

It seems that the case lines in the function are intended to be in the
ATT_*'s definition order, but some of the them are out of that
order. However, I didn't reorder existing lines in the attached. I
didn't check the value itself is correct for the callers.

regareds.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: ERROR: "ft1" is of the wrong type.

    Michael Paquier <michael@paquier.xyz> — 2021-02-18T07:27:23Z

    On Tue, Feb 16, 2021 at 06:14:15PM +0900, Kyotaro Horiguchi wrote:
> The attached is just fixing that.  I tried to make it generic but
> didn't find a clean and translatable way.
> 
> Also I found that only three cases in the function are excecised by
> make check.
> 
> ATT_TABLE                     : foreign_data, indexing checks 
> ATT_TABLE | ATT_FOREIGN_TABLE : alter_table
> ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table
> 
> I'm not sure it's worth the trouble so the attached doesn't do
> anything for that.

Each sentence needs to be completely separate, as the language
translated to may tweak the punctuation of the set of objects listed,
at least.  But you know that already :)

If you have seen cases where permission checks show up messages with
an incorrect relkind mentioned, could you add some regression tests
able to trigger the problematic cases you saw and to improve this
coverage?
--
Michael

  3. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-18T08:17:37Z

    At Thu, 18 Feb 2021 16:27:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Tue, Feb 16, 2021 at 06:14:15PM +0900, Kyotaro Horiguchi wrote:
> > The attached is just fixing that.  I tried to make it generic but
> > didn't find a clean and translatable way.
> > 
> > Also I found that only three cases in the function are excecised by
> > make check.
> > 
> > ATT_TABLE                     : foreign_data, indexing checks 
> > ATT_TABLE | ATT_FOREIGN_TABLE : alter_table
> > ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table
> > 
> > I'm not sure it's worth the trouble so the attached doesn't do
> > anything for that.
> 
> Each sentence needs to be completely separate, as the language
> translated to may tweak the punctuation of the set of objects listed,
> at least.  But you know that already :)

Yeah, I strongly feel that:p As you pointed, the puctuations and the
article (for index and others) was that.

> If you have seen cases where permission checks show up messages with
> an incorrect relkind mentioned, could you add some regression tests
> able to trigger the problematic cases you saw and to improve this
> coverage?

I can add some regression tests to cover all the live cases. That
could reveal no-longer-used combinations.

I'll do that. Thaks for the suggestion.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-19T08:30:39Z

    At Thu, 18 Feb 2021 17:17:37 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> I can add some regression tests to cover all the live cases. That
> could reveal no-longer-used combinations.

The attached is that.

ATT_VIEW is used for "CREATE OR REPLACE view" and checked against
earlier in DefineVirtualRelation. But we can add a test to make sure
that is checked anywhere.

All other values can be exercised.

ATT_TABLE | ATT_MATVIEW
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX |
     ATT_FOREIGN_TABLE
ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_PARTITIONED_INDEX
ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX
ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
ATT_FOREIGN_TABLE

These are provoked by the following commands respectively:

  ALTER TABLE <view> CLUSTER ON
  ALTER TABLE <view> SET TABLESPACE
  ALTER TABLE <view> ALTER COLUMN <col> SET STATISTICS
  ALTER TABLE <view> ALTER COLUMN <col> SET STORGE
  ALTER TABLE <view> ALTER COLUMN <col> SET()
  ALTER TABLE <view> ATTACH PARTITION
  ALTER TABLE/INDEX <partidx> SET/RESET
  ALTER TABLE <matview> ALTER <col> SET DEFAULT
  ALTER TABLE/INDEX <pidx> ALTER COLLATION ..REFRESH VERSION
  ALTER TABLE <view> OPTIONS ()

The following three errors are already excised.

ATT_TABLE
ATT_TABLE | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE:


By the way, I find this as somewhat mystifying. I'm not sure it worth
fixing though..

ALTER MATERIALIZED VIEW mv1 ALTER COLUMN a SET DEFAULT 1;
ERROR:  "mv1" is not a table, view, or foreign table

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: ERROR: "ft1" is of the wrong type.

    Michael Paquier <michael@paquier.xyz> — 2021-04-22T04:48:45Z

    Hi Horiguchi-san,

On Fri, Feb 19, 2021 at 05:30:39PM +0900, Kyotaro Horiguchi wrote:
> The attached is that.
> 
> ATT_VIEW is used for "CREATE OR REPLACE view" and checked against
> earlier in DefineVirtualRelation. But we can add a test to make sure
> that is checked anywhere.

My apologies for not coming back to this thread earlier.  I have this
thread in my backlog for some time now but I was not able to come back
to it.  That's too late for v14 but it could be possible to do
something for v15.  Could you add this patch to the next commit fest?
That's fine to add my name as reviewer.

Thanks,
--
Michael

  6. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-04-22T06:00:55Z

    At Thu, 22 Apr 2021 13:48:45 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> Hi Horiguchi-san,
> 
> On Fri, Feb 19, 2021 at 05:30:39PM +0900, Kyotaro Horiguchi wrote:
> > The attached is that.
> > 
> > ATT_VIEW is used for "CREATE OR REPLACE view" and checked against
> > earlier in DefineVirtualRelation. But we can add a test to make sure
> > that is checked anywhere.
> 
> My apologies for not coming back to this thread earlier.  I have this
> thread in my backlog for some time now but I was not able to come back
> to it.  That's too late for v14 but it could be possible to do
> something for v15.  Could you add this patch to the next commit fest?
> That's fine to add my name as reviewer.

Thank you for kindly telling me that, but please don't worry.

I'll add it to the next CF, with specifying you as a reviewer as you
told.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. Re: ERROR: "ft1" is of the wrong type.

    Ahsan Hadi <ahsan.hadi@gmail.com> — 2021-06-29T20:13:14Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

I have tested it with various object types and getting a meaningful error.

  8. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-06-30T00:55:58Z

    At Tue, 29 Jun 2021 20:13:14 +0000, ahsan hadi <ahsan.hadi@gmail.com> wrote in 
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            not tested
> 
> I have tested it with various object types and getting a meaningful error.

Thanks for looking this, Ahsan.

However, Peter-E is proposing a change at a fundamental level, which
looks more promising (disregarding backpatch burden).

https://www.postgresql.org/message-id/01d4fd55-d4fe-5afc-446c-a7f99e043f3d@enterprisedb.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  9. Re: ERROR: "ft1" is of the wrong type.

    Ahsan Hadi <ahsan.hadi@gmail.com> — 2021-06-30T08:43:52Z

    On Wed, Jun 30, 2021 at 5:56 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

> At Tue, 29 Jun 2021 20:13:14 +0000, ahsan hadi <ahsan.hadi@gmail.com>
> wrote in
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, passed
> > Implements feature:       tested, passed
> > Spec compliant:           tested, passed
> > Documentation:            not tested
> >
> > I have tested it with various object types and getting a meaningful
> error.
>
> Thanks for looking this, Ahsan.
>
> However, Peter-E is proposing a change at a fundamental level, which
> looks more promising (disregarding backpatch burden).
>
>
> https://www.postgresql.org/message-id/01d4fd55-d4fe-5afc-446c-a7f99e043f3d@enterprisedb.com


Sure I will also take a look at this patch.

+1 for avoiding the backpatching burden.


>
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>


-- 
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.hadi@highgo.ca

  10. Re: ERROR: "ft1" is of the wrong type.

    Michael Paquier <michael@paquier.xyz> — 2021-07-02T04:20:21Z

    On Wed, Jun 30, 2021 at 01:43:52PM +0500, Ahsan Hadi wrote:
> Sure I will also take a look at this patch.
> 
> +1 for avoiding the backpatching burden.

From what I recall of this thread, nobody has really complained about
this stuff either, so a backpatch would be off the table.  I agree
that what Peter E is proposing on the other thread is much more
suitable in the long term, as there is no need to worry about multiple
combinations of relkinds in error message, so such error strings
become a no-brainer when more relkinds are added.
--
Michael

  11. Re: ERROR: "ft1" is of the wrong type.

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-08T08:02:53Z

    On 02.07.21 06:20, Michael Paquier wrote:
> On Wed, Jun 30, 2021 at 01:43:52PM +0500, Ahsan Hadi wrote:
>> Sure I will also take a look at this patch.
>>
>> +1 for avoiding the backpatching burden.
> 
>  From what I recall of this thread, nobody has really complained about
> this stuff either, so a backpatch would be off the table.  I agree
> that what Peter E is proposing on the other thread is much more
> suitable in the long term, as there is no need to worry about multiple
> combinations of relkinds in error message, so such error strings
> become a no-brainer when more relkinds are added.

My patch is now committed.  The issue that started this thread now 
behaves like this:

ALTER TABLE ft1 ATTACH PARTITION ...;
ERROR:  ALTER action ATTACH PARTITION cannot be performed on relation "ft1"
DETAIL:  This operation is not supported for foreign tables.

So, for PG15, this is taken care of.

Backpatches under the old style for missing combinations would still be 
in scope, but there my comment on the proposed patches is that I would 
rather not remove apparently unused combinations from back branches.

  12. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-09T01:44:13Z

    At Thu, 8 Jul 2021 10:02:53 +0200, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in 
> My patch is now committed.  The issue that started this thread now behaves
> like this:
> 
> ALTER TABLE ft1 ATTACH PARTITION ...;
> ERROR:  ALTER action ATTACH PARTITION cannot be performed on relation "ft1"
> DETAIL:  This operation is not supported for foreign tables.
> 
> So, for PG15, this is taken care of.

Cool.

> Backpatches under the old style for missing combinations would still be in
> scope, but there my comment on the proposed patches is that I would rather not
> remove apparently unused combinations from back branches.

Sounds reasonable. So the attached are that for PG11-PG14.  11 and 12
shares the same patch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: ERROR: "ft1" is of the wrong type.

    Michael Paquier <michael@paquier.xyz> — 2021-07-09T02:03:56Z

    On Fri, Jul 09, 2021 at 10:44:13AM +0900, Kyotaro Horiguchi wrote:
> Sounds reasonable. So the attached are that for PG11-PG14.  11 and 12
> shares the same patch.

How much do the regression tests published upthread in
https://postgr.es/m/20210219.173039.609314751334535042.horikyota.ntt@gmail.com
apply here?  Shouldn't we also have some regression tests for the new
error cases you are adding?  I agree that we'd better avoid removing
those entries, one argument in favor of not removing any entries being
that this could have an impact on forks.
--
Michael

  14. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-09T12:00:31Z

    At Fri, 9 Jul 2021 11:03:56 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Jul 09, 2021 at 10:44:13AM +0900, Kyotaro Horiguchi wrote:
> > Sounds reasonable. So the attached are that for PG11-PG14.  11 and 12
> > shares the same patch.
> 
> How much do the regression tests published upthread in
> https://postgr.es/m/20210219.173039.609314751334535042.horikyota.ntt@gmail.com
> apply here?  Shouldn't we also have some regression tests for the new
> error cases you are adding?  I agree that we'd better avoid removing

Mmm. Ok, I distributed the mother regression test into each version.

PG11, 12:

 - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX

   Added.

 - ATT_TABLE | ATT_PARTITIONED_INDEX

   This test doesn't detect the "is of the wrong type" issue.

   The item is practically a dead one since the combination is caught
   by transformPartitionCmd before visiting ATPrepCmd, which emits a
   bit different error message for the test.

     "\"%s\" is not a partitioned table or index"

   ATPrepCmd emits an error that:

     "\"%s\" is not a table or partitioned index"

   Hmm.. somewhat funny.  Actually ATT_TABLE is a bit off here but
   there's no symbol ATT_PARTITIONED_TABLE.  Theoretically the symbol
   is needed but practically not.  I don't think we need to do more
   than that at least for these versions.  (Or we don't even need to
   add this item.)

PG13:

 - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX

   Same to PG12.

 - ATT_TABLE | ATT_PARTITIONED_INDEX:

   This version raches this item in ATPrepCmd because the commit
   1281a5c907 moved the parse-transform phase to the ATExec stage,
   which is visited after ATPrepCmd.

   On the other hand, when the target relation is a regular table, the
   error is missed by ATPrepCmd then the control reaches to the
   Exec-stage. The error is finally aught by transformPartitionCmd.

   Of course this works fine but doesn't seem clean, but it is
   apparently a matter of the master branch.

 - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
   Added and works as expected.

PG14:

 - ATT_INDEX

   I noticed that this combination has been reverted by the commit
   ec48314708.

 - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
 - ATT_TABLE | ATT_PARTITIONED_INDEX:
 - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE

   Same as PG13.

   So, PG14 and 13 share the same fix and test.

> error cases you are adding?  I agree that we'd better avoid removing
> those entries, one argument in favor of not removing any entries being
> that this could have an impact on forks.

Ok. The attached are the two patchsets for PG14-13 and PG12-11
containing the fix and the regression test.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: ERROR: "ft1" is of the wrong type.

    Michael Paquier <michael@paquier.xyz> — 2021-07-14T10:55:18Z

    On Fri, Jul 09, 2021 at 09:00:31PM +0900, Kyotaro Horiguchi wrote:
> Mmm. Ok, I distributed the mother regression test into each version.

Thanks, my apologies for the late reply.  It took me some time to
analyze the whole.

> PG11, 12:
>
>  - ATT_TABLE | ATT_PARTITIONED_INDEX
> 
>    This test doesn't detect the "is of the wrong type" issue.
> 
>    The item is practically a dead one since the combination is caught
>    by transformPartitionCmd before visiting ATPrepCmd, which emits a
>    bit different error message for the test.

Yes, I was surprised to see this test choke in the utility parsing.
There is a good argument in keeping (ATT_TABLE |
ATT_PARTITIONED_INDEX) though.  I analyzed the code and I agree that
it cannot be directly reached, but a future code change on those
branches may expose that.  And it does not really cost in keeping it
either.

> PG13:
>    Of course this works fine but doesn't seem clean, but it is
>    apparently a matter of the master branch.
> 
>  - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
>    Added and works as expected.

HEAD had its own improvements, and what you have here closes some
holes of their own, so applied.  Thanks!
--
Michael

  16. Re: ERROR: "ft1" is of the wrong type.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-15T04:51:31Z

    At Wed, 14 Jul 2021 19:55:18 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Jul 09, 2021 at 09:00:31PM +0900, Kyotaro Horiguchi wrote:
> > Mmm. Ok, I distributed the mother regression test into each version.
> 
> Thanks, my apologies for the late reply.  It took me some time to
> analyze the whole.
..
> > PG13:
> >    Of course this works fine but doesn't seem clean, but it is
> >    apparently a matter of the master branch.
> > 
> >  - ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
> >    Added and works as expected.
> 
> HEAD had its own improvements, and what you have here closes some
> holes of their own, so applied.  Thanks!

Thank you for commiting this!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center