ERROR: "ft1" is of the wrong type.

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

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

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

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

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

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.

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).

/messages/by-id/01d4fd55-d4fe-5afc-446c-a7f99e043f3d@enterprisedb.com

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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).

/messages/by-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

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

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.

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

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
/messages/by-id/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

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
/messages/by-id/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

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

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