Thread: [HACKERS] "create publication..all tables" ignore 'partition not supported'error

Hi,

I observed that - "create publication..all tables" ignore 'partition not 
supported' error

\\create a partition table

You are now connected to database "s" as user "centos".
s=# CREATE TABLE measurement (
s(#     city_id         int not null,
s(#     logdate         date not null,
s(#     peaktemp        int,
s(#     unitsales       int
s(# ) PARTITION BY RANGE (logdate);
CREATE TABLE
s=#

\\try to publish only this table

s=# create publication p for table  measurement;
ERROR:  "measurement" is a partitioned table
DETAIL:  Adding partitioned tables to publications is not supported.
HINT:  You can add the table partitions individually.

\\try to create publication for all tables
s=# create publication p for all tables ;
CREATE PUBLICATION
s=# \d+ measurement                                 Table "public.measurement"  Column   |  Type   | Collation |
Nullable| Default | Storage | Stats 
 
target | Description
-----------+---------+-----------+----------+---------+---------+--------------+------------- city_id   | integer |
     | not null |         | plain 
 
|              | logdate   | date    |           | not null |         | plain 
|              | peaktemp  | integer |           |          |         | plain 
|              | unitsales | integer |           |          |         | plain 
|              |
Partition key: RANGE (logdate)
Publications:    "p"

Publication 'p' has been set against partition table ,which is not 
supported.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company




Yeah, it's a bug. While showing the table definition, we use the
following query for showing the related publications:
"SELECT pub.pubname\n"                             " FROM pg_catalog.pg_publication pub\n"
"LEFT JOIN pg_catalog.pg_publication_rel pr\n"                             "      ON (pr.prpubid = pub.oid)\n"
                  "WHERE pr.prrelid = '%s' OR pub.puballtables\n"                             "ORDER BY 1;"
 

When pub.puballtables is TRUE, we should also check whether the
relation is publishable or not.(Something like is_publishable_class
method in pg_publication.c).

However, I'm not sure whether this is the correct way to solve the problem.

On Mon, May 22, 2017 at 2:39 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:
> Hi,
>
> I observed that - "create publication..all tables" ignore 'partition not
> supported' error
>
> \\create a partition table
>
> You are now connected to database "s" as user "centos".
> s=# CREATE TABLE measurement (
> s(#     city_id         int not null,
> s(#     logdate         date not null,
> s(#     peaktemp        int,
> s(#     unitsales       int
> s(# ) PARTITION BY RANGE (logdate);
> CREATE TABLE
> s=#
>
> \\try to publish only this table
>
> s=# create publication p for table  measurement;
> ERROR:  "measurement" is a partitioned table
> DETAIL:  Adding partitioned tables to publications is not supported.
> HINT:  You can add the table partitions individually.
>
> \\try to create publication for all tables
> s=# create publication p for all tables ;
> CREATE PUBLICATION
> s=# \d+ measurement
>                                  Table "public.measurement"
>   Column   |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> -----------+---------+-----------+----------+---------+---------+--------------+-------------
>  city_id   | integer |           | not null |         | plain |
> |
>  logdate   | date    |           | not null |         | plain |
> |
>  peaktemp  | integer |           |          |         | plain |
> |
>  unitsales | integer |           |          |         | plain |
> |
> Partition key: RANGE (logdate)
> Publications:
>     "p"
>
> Publication 'p' has been set against partition table ,which is not
> supported.
>
> --
> regards,tushar
> EnterpriseDB  https://www.enterprisedb.com/
> The Enterprise PostgreSQL Company
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com



On 2017/05/22 20:02, Kuntal Ghosh wrote:
> Yeah, it's a bug. While showing the table definition, we use the
> following query for showing the related publications:
> "SELECT pub.pubname\n"
>                               " FROM pg_catalog.pg_publication pub\n"
>                               " LEFT JOIN pg_catalog.pg_publication_rel pr\n"
>                               "      ON (pr.prpubid = pub.oid)\n"
>                               "WHERE pr.prrelid = '%s' OR pub.puballtables\n"
>                               "ORDER BY 1;"
> 
> When pub.puballtables is TRUE, we should also check whether the
> relation is publishable or not.(Something like is_publishable_class
> method in pg_publication.c).

BTW, you can see the following too, because of the quoted query:

create publication pub2 for all tables;

-- system tables are not publishable, but...
\d pg_class
<snip>
Publications:   "pub2"

-- unlogged tables are not publishable, but...
create unlogged table foo (a int);
\d foo
<snip>
Publications:   "pub2"

-- temp tables are not publishable, but...
create temp table bar (a int);
\d bar
<snip>
Publications:   "pub2"

The above contradicts what check_publication_add_tables() thinks are
publishable relations.

At first, I thought this would be fixed by simply adding a check on
relkind and relpersistence in the above query, both of which are available
to describeOneTableDetails() already.  But, it won't be possible to check
the system table status using the available information, so we might need
to add a SQL-callable version of is_publishable_class() after all.

> 
> However, I'm not sure whether this is the correct way to solve the problem.

AFAICS, the problem is with psql's describe query itself and no other
parts of the system are affected by this.  So, fixing the query after
adding appropriate backend support will do, I think.

Thanks,
Amit




On Tue, May 30, 2017 at 1:42 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/05/22 20:02, Kuntal Ghosh wrote:
>> Yeah, it's a bug. While showing the table definition, we use the
>> following query for showing the related publications:
>> "SELECT pub.pubname\n"
>>                               " FROM pg_catalog.pg_publication pub\n"
>>                               " LEFT JOIN pg_catalog.pg_publication_rel pr\n"
>>                               "      ON (pr.prpubid = pub.oid)\n"
>>                               "WHERE pr.prrelid = '%s' OR pub.puballtables\n"
>>                               "ORDER BY 1;"
>>
>> When pub.puballtables is TRUE, we should also check whether the
>> relation is publishable or not.(Something like is_publishable_class
>> method in pg_publication.c).
>
> BTW, you can see the following too, because of the quoted query:
>
> create publication pub2 for all tables;
>
> -- system tables are not publishable, but...
> \d pg_class
> <snip>
> Publications:
>     "pub2"
>
> -- unlogged tables are not publishable, but...
> create unlogged table foo (a int);
> \d foo
> <snip>
> Publications:
>     "pub2"
>
> -- temp tables are not publishable, but...
> create temp table bar (a int);
> \d bar
> <snip>
> Publications:
>     "pub2"
>
> The above contradicts what check_publication_add_tables() thinks are
> publishable relations.
>
> At first, I thought this would be fixed by simply adding a check on
> relkind and relpersistence in the above query, both of which are available
> to describeOneTableDetails() already.  But, it won't be possible to check
> the system table status using the available information, so we might need
> to add a SQL-callable version of is_publishable_class() after all.

I'd say we can fix this issue by just changing the query. Attached
patch changes the query so that it can handle publication name
correctly, the query gets complex, though.

>>
>> However, I'm not sure whether this is the correct way to solve the problem.
>
> AFAICS, the problem is with psql's describe query itself and no other
> parts of the system are affected by this.

I think so too.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment
On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I'd say we can fix this issue by just changing the query. Attached
> patch changes the query so that it can handle publication name
> correctly, the query gets complex, though.
>
In is_publishable_class function, there are four conditions to decide
whether this is a publishable class or not.

1. relkind == RELKIND_RELATION
2. IsCatalogClass()
3. relpersistence == 'p'
4. relid >= FirstNormalObjectId /* Skip tables created during initdb */

I think the modified query should have a check for the fourth condition as well.

Added to open items.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com



On 5/31/17 02:17, Kuntal Ghosh wrote:
> On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>
>> I'd say we can fix this issue by just changing the query. Attached
>> patch changes the query so that it can handle publication name
>> correctly, the query gets complex, though.
>>
> In is_publishable_class function, there are four conditions to decide
> whether this is a publishable class or not.
> 
> 1. relkind == RELKIND_RELATION
> 2. IsCatalogClass()
> 3. relpersistence == 'p'
> 4. relid >= FirstNormalObjectId /* Skip tables created during initdb */
> 
> I think the modified query should have a check for the fourth condition as well.

The query should be fixed like in the attached patch.
pg_get_publication_tables() ends up calling is_publishable_class()
internally.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment
On 2017/06/01 10:26, Peter Eisentraut wrote:
> On 5/31/17 02:17, Kuntal Ghosh wrote:
>> On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>
>>> I'd say we can fix this issue by just changing the query. Attached
>>> patch changes the query so that it can handle publication name
>>> correctly, the query gets complex, though.
>>>
>> In is_publishable_class function, there are four conditions to decide
>> whether this is a publishable class or not.
>>
>> 1. relkind == RELKIND_RELATION
>> 2. IsCatalogClass()
>> 3. relpersistence == 'p'
>> 4. relid >= FirstNormalObjectId /* Skip tables created during initdb */
>>
>> I think the modified query should have a check for the fourth condition as well.
> 
> The query should be fixed like in the attached patch.
> pg_get_publication_tables() ends up calling is_publishable_class()
> internally.

Looks good to me.

Thanks,
Amit




On Thu, Jun 1, 2017 at 6:56 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/31/17 02:17, Kuntal Ghosh wrote:
>> On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>
>>> I'd say we can fix this issue by just changing the query. Attached
>>> patch changes the query so that it can handle publication name
>>> correctly, the query gets complex, though.
>>>
>> In is_publishable_class function, there are four conditions to decide
>> whether this is a publishable class or not.
>>
>> 1. relkind == RELKIND_RELATION
>> 2. IsCatalogClass()
>> 3. relpersistence == 'p'
>> 4. relid >= FirstNormalObjectId /* Skip tables created during initdb */
>>
>> I think the modified query should have a check for the fourth condition as well.
>
> The query should be fixed like in the attached patch.
> pg_get_publication_tables() ends up calling is_publishable_class()
> internally.
>
Works fine.



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com



On 5/31/17 21:26, Peter Eisentraut wrote:
> On 5/31/17 02:17, Kuntal Ghosh wrote:
>> On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>
>>> I'd say we can fix this issue by just changing the query. Attached
>>> patch changes the query so that it can handle publication name
>>> correctly, the query gets complex, though.
>>>
>> In is_publishable_class function, there are four conditions to decide
>> whether this is a publishable class or not.
>>
>> 1. relkind == RELKIND_RELATION
>> 2. IsCatalogClass()
>> 3. relpersistence == 'p'
>> 4. relid >= FirstNormalObjectId /* Skip tables created during initdb */
>>
>> I think the modified query should have a check for the fourth condition as well.
> 
> The query should be fixed like in the attached patch.
> pg_get_publication_tables() ends up calling is_publishable_class()
> internally.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services