Thread: Error for WITH options on partitioned tables

Someone on general list recently complained that the error message
from trying to use options on a partitioned table was misleading,
which it definitely is:

CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a)
WITH (fillfactor=100);
ERROR:  unrecognized parameter "fillfactor"

Which is verified by patch 001.

Patch 002 replaces this with a more meaningful error message, which
matches our fine manual.
https://www.postgresql.org/docs/current/sql-createtable.html

 ERROR:  cannot specify storage options for a partitioned table
 HINT:  specify storage options on leaf partitions instead

-- 
Simon Riggs                http://www.EnterpriseDB.com/

On Fri, 16 Sep 2022 at 20:13, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
> Someone on general list recently complained that the error message
> from trying to use options on a partitioned table was misleading,
> which it definitely is:
>
> CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a)
> WITH (fillfactor=100);
> ERROR:  unrecognized parameter "fillfactor"
>
> Which is verified by patch 001.
>
> Patch 002 replaces this with a more meaningful error message, which
> matches our fine manual.
> https://www.postgresql.org/docs/current/sql-createtable.html
>
>  ERROR:  cannot specify storage options for a partitioned table
>  HINT:  specify storage options on leaf partitions instead

Looks good.  Does this means we don't need the partitioned_table_reloptions()
function and remove the reloptions validation in DefineRelation() for
partitioned table.  Or we can ereport() in partitioned_table_reloptions().

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

I wrote:
> On Fri, 16 Sep 2022 at 20:13, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>> Patch 002 replaces this with a more meaningful error message, which
>> matches our fine manual.
>> https://www.postgresql.org/docs/current/sql-createtable.html
>>
>>  ERROR:  cannot specify storage options for a partitioned table
>>  HINT:  specify storage options on leaf partitions instead
>
> Looks good.  Does this means we don't need the partitioned_table_reloptions()
> function and remove the reloptions validation in DefineRelation() for
> partitioned table.  Or we can ereport() in partitioned_table_reloptions().

I want to know why we should do validation for partitioned tables even if it
doesn't support storage parameters?

    /*
     * There are no options for partitioned tables yet, but this is able to do
     * some validation.
     */
    return (bytea *) build_reloptions(reloptions, validate,
                                      RELOPT_KIND_PARTITIONED,
                                      0, NULL, 0);

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Hi Karina,

I am not very clear about why `build_reloptions` is removed in patch 
`v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if 
you can help explain would be great.

 From my observation, it seems the WITH option has different behavior 
when creating partitioned table and index. For example,

pgbench -i --partitions=2 --partition-method=range -d postgres

postgres=# create index idx_bid on pgbench_accounts using btree(bid) 
with (fillfactor = 90);
CREATE INDEX

postgres=# select relname, relkind, reloptions from pg_class where 
relnamespace=2200 order by oid;
           relname           | relkind |    reloptions
----------------------------+---------+------------------
  idx_bid                    | I       | {fillfactor=90}
  pgbench_accounts_1_bid_idx | i       | {fillfactor=90}
  pgbench_accounts_2_bid_idx | i       | {fillfactor=90}

I can see the `fillfactor` parameter has been added to the indexes, 
however, if I try to alter `fillfactor`, I got an error like below.
postgres=# alter index idx_bid set (fillfactor=40);
ERROR:  ALTER action SET cannot be performed on relation "idx_bid"
DETAIL:  This operation is not supported for partitioned indexes.

This generic error message is reported by 
`errdetail_relkind_not_supported`, and there is a similar error message 
for partitioned tables. Anyone knows if this can be an option for 
reporting this `fillfactor` parameter not supported error.


Best regards,

David

On 2022-10-14 8:16 a.m., Karina Litskevich wrote:
> Hi, Simon!
>
> The new error message looks better. But I believe it is better to use
> "parameters" instead of "options" as it is called "storage parameters"
> in the documentation. I also believe it is better to report error in
> partitioned_table_reloptions() (thanks to Japin Li for mentioning this
> function) as it also fixes the error message in the following situation:
>
> test=# CREATE TABLE parted_col_comment (a int, b text) PARTITION BY 
> LIST (a);
> CREATE TABLE
> test=# ALTER TABLE parted_col_comment SET (fillfactor=100);
> ERROR:  unrecognized parameter "fillfactor"
>
> I attached the new versions of patches.
>
> I'm not sure about the errcode. May be it is better to report error with
> ERRCODE_INVALID_OBJECT_DEFINITION for CREATE TABLE and with
> ERRCODE_WRONG_OBJECT_TYPE for ALTER TABLE (as when you try "ALTER TABLE
> partitioned INHERIT nonpartitioned;" an ERROR with 
> ERRCODE_WRONG_OBJECT_TYPE
> is reported). Then either the desired code should be passed to
> partitioned_table_reloptions() or similar checks and ereport calls 
> should be
> placed in two different places. I'm not sure whether it is a good idea to
> change the code in one of these ways just to change the error code.
>
> Best regards,
> Karina Litskevich
> Postgres Professional: http://postgrespro.com/

Apologies, I only just noticed these messages. I have set WoA until I
have read, understood and can respond to your detailed input, thanks.

On Fri, 28 Oct 2022 at 22:21, David Zhang <david.zhang@highgo.ca> wrote:
>
> Hi Karina,
>
> I am not very clear about why `build_reloptions` is removed in patch
> `v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if
> you can help explain would be great.
>
>  From my observation, it seems the WITH option has different behavior
> when creating partitioned table and index. For example,
>
> pgbench -i --partitions=2 --partition-method=range -d postgres
>
> postgres=# create index idx_bid on pgbench_accounts using btree(bid)
> with (fillfactor = 90);
> CREATE INDEX
>
> postgres=# select relname, relkind, reloptions from pg_class where
> relnamespace=2200 order by oid;
>            relname           | relkind |    reloptions
> ----------------------------+---------+------------------
>   idx_bid                    | I       | {fillfactor=90}
>   pgbench_accounts_1_bid_idx | i       | {fillfactor=90}
>   pgbench_accounts_2_bid_idx | i       | {fillfactor=90}
>
> I can see the `fillfactor` parameter has been added to the indexes,
> however, if I try to alter `fillfactor`, I got an error like below.
> postgres=# alter index idx_bid set (fillfactor=40);
> ERROR:  ALTER action SET cannot be performed on relation "idx_bid"
> DETAIL:  This operation is not supported for partitioned indexes.
>
> This generic error message is reported by
> `errdetail_relkind_not_supported`, and there is a similar error message
> for partitioned tables. Anyone knows if this can be an option for
> reporting this `fillfactor` parameter not supported error.
>
>
> Best regards,
>
> David
>
> On 2022-10-14 8:16 a.m., Karina Litskevich wrote:
> > Hi, Simon!
> >
> > The new error message looks better. But I believe it is better to use
> > "parameters" instead of "options" as it is called "storage parameters"
> > in the documentation. I also believe it is better to report error in
> > partitioned_table_reloptions() (thanks to Japin Li for mentioning this
> > function) as it also fixes the error message in the following situation:
> >
> > test=# CREATE TABLE parted_col_comment (a int, b text) PARTITION BY
> > LIST (a);
> > CREATE TABLE
> > test=# ALTER TABLE parted_col_comment SET (fillfactor=100);
> > ERROR:  unrecognized parameter "fillfactor"
> >
> > I attached the new versions of patches.
> >
> > I'm not sure about the errcode. May be it is better to report error with
> > ERRCODE_INVALID_OBJECT_DEFINITION for CREATE TABLE and with
> > ERRCODE_WRONG_OBJECT_TYPE for ALTER TABLE (as when you try "ALTER TABLE
> > partitioned INHERIT nonpartitioned;" an ERROR with
> > ERRCODE_WRONG_OBJECT_TYPE
> > is reported). Then either the desired code should be passed to
> > partitioned_table_reloptions() or similar checks and ereport calls
> > should be
> > placed in two different places. I'm not sure whether it is a good idea to
> > change the code in one of these ways just to change the error code.
> >
> > Best regards,
> > Karina Litskevich
> > Postgres Professional: http://postgrespro.com/
>


-- 
Simon Riggs                http://www.EnterpriseDB.com/

On Mon, 7 Nov 2022 at 08:55, Karina Litskevich
<litskevichkarina@gmail.com> wrote:
>
> Hi David,
>
> > I am not very clear about why `build_reloptions` is removed in patch
> > `v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if
> > you can help explain would be great.
>
> "build_reloptions" parses "reloptions" and takes for it a list of allowed
> options defined by the 5th argument "relopt_elems" and the 6th argument
> "num_relopt_elems", which are NULL and 0 in the removed call. If "validate"
> is false, it ignores options, which are not in the list, while parsing. If
> "validate" is true, it "elog"s ERROR when it meets option, which is not in the
> allowed list.

Karina's changes make sense to me, so +1.

This is a minor patch, so I will set this as Ready For Committer.

-- 
Simon Riggs                http://www.EnterpriseDB.com/

Simon Riggs <simon.riggs@enterprisedb.com> writes:
> Karina's changes make sense to me, so +1.
> This is a minor patch, so I will set this as Ready For Committer.

Pushed with minor fiddling:

* I concur with Karina's thought that ERRCODE_WRONG_OBJECT_TYPE
is the most on-point errcode for this.  The complaint is specifically
about the table relkind and has nothing to do with the storage
parameter per se.  I also agree that it's not worth trying to use
a different errcode for CREATE vs. ALTER.

* The HINT message wasn't per project style (it should be formatted as
a complete sentence), and I thought using "parameters for" in the
main message but "parameters on" in the hint was oddly inconsistent.

            regards, tom lane