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

2017-06-01 Thread Peter Eisentraut 
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  
>> 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


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

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

2017-06-01 Thread Kuntal Ghosh 
On Thu, Jun 1, 2017 at 6:56 AM, Peter Eisentraut
 wrote:
> On 5/31/17 02:17, Kuntal Ghosh wrote:
>> On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada  
>> 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


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

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

2017-05-31 Thread Amit Langote 
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  
>> 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



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

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

2017-05-31 Thread Peter Eisentraut 
On 5/31/17 02:17, Kuntal Ghosh wrote:
> On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada  
> 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
From 4a93e9b8432661724e9fd400c8f267dd09d3b513 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 31 May 2017 21:23:06 -0400
Subject: [PATCH] psql: Fix display of whether table is part of publication

If a FOR ALL TABLES publication was present, \d of a table would claim
for each table that it was part of the publication, even for tables that
are ignores, such as system tables and unlogged tables.  Fix the query
by using the function pg_get_publication_tables(), which was intended for
this purpose.

Reported-by: tushar 
---
 src/bin/psql/describe.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 3e542f7b1d..f1c3d9b7e0 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2537,10 +2537,9 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(,
  "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"
+ " FROM 
pg_catalog.pg_publication pub,\n"
+ "  
pg_catalog.pg_get_publication_tables(pub.pubname)\n"
+ "WHERE relid = '%s'\n"
  "ORDER BY 1;",
  oid);
 
-- 
2.13.0


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

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

2017-05-31 Thread Kuntal Ghosh 
On Wed, May 31, 2017 at 12:58 AM, Masahiko Sawada  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


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

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

2017-05-30 Thread Masahiko Sawada 
On Tue, May 30, 2017 at 1:42 PM, Amit Langote
 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
> 
> Publications:
> "pub2"
>
> -- unlogged tables are not publishable, but...
> create unlogged table foo (a int);
> \d foo
> 
> Publications:
> "pub2"
>
> -- temp tables are not publishable, but...
> create temp table bar (a int);
> \d bar
> 
> 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


psql_publication.patch
Description: Binary data

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

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

2017-05-29 Thread Amit Langote 
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

Publications:
"pub2"

-- unlogged tables are not publishable, but...
create unlogged table foo (a int);
\d foo

Publications:
"pub2"

-- temp tables are not publishable, but...
create temp table bar (a int);
\d bar

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



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

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

2017-05-22 Thread Kuntal Ghosh 
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  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(# peaktempint,
> 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


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

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

2017-05-22 Thread tushar 

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(# peaktempint,
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