Re: odd behavior/possible bug (Was: Re: [HACKERS] PG10 partitioning - odd behavior/possible bug)

[Joe Conway]

On 09/03/2017 03:34 PM, Tom Lane wrote:
> Joe Conway  writes:
>> Notice that tsr is not empty at all on the first loop, but on the second
>> loop it is empty every second time the trigger fires.
> 
> I think the issue is that now() isn't changing within the transaction,
> so when you construct "tstzrange(lower(OLD.tr), now(), '[)')" using an
> old row whose "lower(OLD.tr)" is already "now()", you get an empty range.
> Probably using '[]' bounds would avoid the oddness.

Hmmm, good point. Sorry for the noise.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: odd behavior/possible bug (Was: Re: [HACKERS] PG10 partitioning - odd behavior/possible bug)

[Tom Lane]

Joe Conway  writes:
> Notice that tsr is not empty at all on the first loop, but on the second
> loop it is empty every second time the trigger fires.

I think the issue is that now() isn't changing within the transaction,
so when you construct "tstzrange(lower(OLD.tr), now(), '[)')" using an
old row whose "lower(OLD.tr)" is already "now()", you get an empty range.
Probably using '[]' bounds would avoid the oddness.

regards, tom lane


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

odd behavior/possible bug (Was: Re: [HACKERS] PG10 partitioning - odd behavior/possible bug)

[Joe Conway]

On 09/03/2017 02:28 PM, Joe Conway wrote:
> I was playing around with partitioning and found an oddity that is best
> described with the following reasonably minimal test case:



> Notice that in the first loop iteration tsr is calculated from OLD.tr
> correctly. But in the second loop iteration it is not, and therefore no
> partition can be found for the insert.
> 
> I have not dug too deeply into this yet, but was wondering if this
> behavior is sane/expected for some reason I am missing?

This does not require partitioning to reproduce -- sorry for the false
accusations ;-)

8<---
CREATE TABLE timetravel
(
  id int8,
  f1 text not null,
  tr tstzrange not null default tstzrange(now(), 'infinity', '[]')
);

CREATE OR REPLACE FUNCTION modify_timetravel()
RETURNS TRIGGER AS $$
  DECLARE
tsr tstzrange;
  BEGIN
RAISE NOTICE 'OLD.tr = %', OLD.tr;

tsr := tstzrange(lower(OLD.tr), now(), '[)');
RAISE NOTICE 'tsr = %', tsr;

OLD.tr = tsr;
INSERT INTO timetravel VALUES (OLD.*);
IF (TG_OP = 'UPDATE') THEN
  tsr := tstzrange(now(), 'infinity', '[]');
  RAISE NOTICE 'NEW.tr = %', tsr;
  NEW.tr = tsr;
  RETURN NEW;
ELSIF (TG_OP = 'DELETE') THEN
  RETURN OLD;
END IF;
  END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER timetravel_audit BEFORE DELETE OR UPDATE
ON timetravel FOR EACH ROW EXECUTE PROCEDURE modify_timetravel();

INSERT INTO timetravel(id, f1)
SELECT g.i, 'row-' || g.i::text
FROM generate_series(1,10) AS g(i);

DO $$
  DECLARE
i int;
  BEGIN
FOR i IN 1..2 LOOP
  RAISE NOTICE 'loop = %', i;
  UPDATE timetravel SET f1 = f1 || '-r' || i where id < 4;
END LOOP;
  END
$$;
NOTICE:  loop = 1
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07",infinity]
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07",infinity]
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07",infinity]
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  loop = 2
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  tsr = empty
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  tsr = empty
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  tsr = empty
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
DO
8<---

Notice that tsr is not empty at all on the first loop, but on the second
loop it is empty every second time the trigger fires.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] PG10 partitioning - odd behavior/possible bug

[Joe Conway]

I was playing around with partitioning and found an oddity that is best
described with the following reasonably minimal test case:

8<-
CREATE TABLE timetravel
(
  id int8,
  f1 text not null,
  tr tstzrange not null default tstzrange(now(), 'infinity', '[]')
) PARTITION BY RANGE (upper(tr));

CREATE TABLE timetravel_current PARTITION OF timetravel
(
  primary key (id, tr) DEFERRABLE
) FOR VALUES FROM ('infinity') TO (MAXVALUE);
CREATE INDEX timetravel_current_tr_idx ON timetravel_current USING GIST
(tr);

CREATE TABLE timetravel_history PARTITION OF timetravel
(
  primary key (id, tr) DEFERRABLE
) FOR VALUES FROM (MINVALUE) TO ('infinity');
CREATE INDEX timetravel_history_tr_idx ON timetravel_history USING GIST
(tr);

CREATE OR REPLACE FUNCTION modify_timetravel()
RETURNS TRIGGER AS $$
  DECLARE
tsr tstzrange;
  BEGIN
RAISE NOTICE 'OLD.tr = %', OLD.tr;

tsr := tstzrange(lower(OLD.tr), now(), '[)');
RAISE NOTICE 'tsr = %', tsr;

OLD.tr = tsr;
INSERT INTO timetravel VALUES (OLD.*);
IF (TG_OP = 'UPDATE') THEN
  tsr := tstzrange(now(), 'infinity', '[]');
  RAISE NOTICE 'NEW.tr = %', tsr;
  NEW.tr = tsr;
  RETURN NEW;
ELSIF (TG_OP = 'DELETE') THEN
  RETURN OLD;
END IF;
  END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER timetravel_audit BEFORE DELETE OR UPDATE
ON timetravel_current FOR EACH ROW EXECUTE PROCEDURE modify_timetravel();

INSERT INTO timetravel(id, f1)
SELECT g.i, 'row-' || g.i::text
FROM generate_series(1,10) AS g(i);

DO $$
  DECLARE
i int;
  BEGIN
FOR i IN 1..2 LOOP
  RAISE NOTICE 'loop = %', i;
  UPDATE timetravel SET f1 = f1 || '-r' || i where id < 2;
END LOOP;
  END
$$;
NOTICE:  loop = 1
NOTICE:  OLD.tr = ["2017-09-03 14:15:08.800811-07",infinity]
NOTICE:  tsr = ["2017-09-03 14:15:08.800811-07","2017-09-03
14:18:48.270274-07")
NOTICE:  NEW.tr = ["2017-09-03 14:18:48.270274-07",infinity]
NOTICE:  loop = 2
NOTICE:  OLD.tr = ["2017-09-03 14:18:48.270274-07",infinity]
NOTICE:  tsr = empty
ERROR:  no partition of relation "timetravel" found for row
DETAIL:  Partition key of the failing row contains (upper(tr)) = (null).
CONTEXT:  SQL statement "INSERT INTO timetravel VALUES (OLD.*)"
PL/pgSQL function modify_timetravel() line 11 at SQL statement
SQL statement "UPDATE timetravel SET f1 = f1 || '-r' || i where id < 2"
PL/pgSQL function inline_code_block line 7 at SQL statement
8<-

Notice that in the first loop iteration tsr is calculated from OLD.tr
correctly. But in the second loop iteration it is not, and therefore no
partition can be found for the insert.

I have not dug too deeply into this yet, but was wondering if this
behavior is sane/expected for some reason I am missing?

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] hash partitioning based on v10Beta2

[yangjie]

Hi,



When the number of partitions and the data are more, adding new partitions, there will be some efficiency problems.


I don't know how the solution you're talking about is how to implement a hash partition?



















---youngHighGo Database: http://www.highgo.com











On 8/28/2017 22:25，Robert Haas wrote： 


On Sat, Aug 26, 2017 at 12:40 AM, yang...@highgo.com  wrote:
> A partition table can be create as bellow:
>
>  CREATE TABLE h1 PARTITION OF h;
>  CREATE TABLE h2 PARTITION OF h;
>  CREATE TABLE h3 PARTITION OF h;

This syntax is very problematic for reasons that have been discussed
on the existing hash partitioning thread.  Fortunately, a solution has
already been implemented... you're the third person to try to write a
patch for this feature.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] hash partitioning based on v10Beta2

[Robert Haas]

On Mon, Aug 28, 2017 at 10:44 PM, yangjie  wrote:
> When the number of partitions and the data are more, adding new partitions,
> there will be some efficiency problems.
> I don't know how the solution you're talking about is how to implement a
> hash partition?

I am having difficulty understanding this.  There was discussion on
the other thread of how splitting partitions could be done reasonably
efficiently with the proposed design; of course, it's never going to
be super-cheap.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] hash partitioning based on v10Beta2

[Robert Haas]

On Sat, Aug 26, 2017 at 12:40 AM, yang...@highgo.com  wrote:
> A partition table can be create as bellow:
>
>  CREATE TABLE h1 PARTITION OF h;
>  CREATE TABLE h2 PARTITION OF h;
>  CREATE TABLE h3 PARTITION OF h;

This syntax is very problematic for reasons that have been discussed
on the existing hash partitioning thread.  Fortunately, a solution has
already been implemented... you're the third person to try to write a
patch for this feature.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] hash partitioning based on v10Beta2

[yang...@highgo.com]

On Sat, Aug 26, 2017 at 10:10 AM, yang...@highgo.com 
wrote:

> Hi all,
>
> Now we have had the range / list partition, but hash partitioning is not
> implemented yet.
> Attached is a POC patch based on the v10Beta2 to add the
> hash partitioning feature.
> Although we will need more discussions about the syntax
> and other specifications before going ahead the project,
> but I think this runnable code might help to discuss
> what and how we implement this.
>
>
FYI, there is already an existing commitfest entry for this project.

https://commitfest.postgresql.org/14/1059/



> Description
>
> The hash partition's implement is on the basis of
> the original range / list partition,and using similar syntax.
>
> To create a partitioned table ,use:
>
> CREATE TABLE h (id int) PARTITION BY HASH(id);
>
> The partitioning key supports only one value, and I think
> the partition key can support multiple values,
> which may be difficult to implement when querying, but
> it is not impossible.
>
> A partition table can be create as bellow:
>
>  CREATE TABLE h1 PARTITION OF h;
>  CREATE TABLE h2 PARTITION OF h;
>  CREATE TABLE h3 PARTITION OF h;
>
> FOR VALUES clause cannot be used, and the partition bound
> is calclulated automatically as partition index of single integer value.
>
> An inserted record is stored in a partition whose index equals
> DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0],
> TYPECACHE_HASH_PROC)->hash_proc, values[0])) % nparts
> /* Number of partitions */
> ;
> In the above example, this is DatumGetUInt32(OidFunctionCall1(lookup_type_
> cache(key->parttypid[0], TYPECACHE_HASH_PROC)->hash_proc, id)) % 3;
>
> postgres=# insert into h select generate_series(1,20);
> INSERT 0 20
> postgres=# select tableoid::regclass,* from h;
>  tableoid | id
> --+
>  h1   |  3
>  h1   |  5
>  h1   | 17
>  h1   | 19
>  h2   |  2
>  h2   |  6
>  h2   |  7
>  h2   | 11
>  h2   | 12
>  h2   | 14
>  h2   | 15
>  h2   | 18
>  h2   | 20
>  h3   |  1
>  h3   |  4
>  h3   |  8
>  h3   |  9
>  h3   | 10
>  h3   | 13
>  h3   | 16
> (20 rows)
>
> The number of partitions here can be dynamically added, and
> if a new partition is created, the number of partitions
> changes, the calculated target partitions will change,
>  and the same data is not reasonable in different
> partitions,So you need to re-calculate the existing data
> and insert the target partition when you create a new partition.
>
> postgres=# create table h4 partition of h;
> CREATE TABLE
> postgres=# select tableoid::regclass,* from h;
>  tableoid | id
> --+
>  h1   |  5
>  h1   | 17
>  h1   | 19
>  h1   |  6
>  h1   | 12
>  h1   |  8
>  h1   | 13
>  h2   | 11
>  h2   | 14
>  h3   |  1
>  h3   |  9
>  h3   |  2
>  h3   | 15
>  h4   |  3
>  h4   |  7
>  h4   | 18
>  h4   | 20
>  h4   |  4
>  h4   | 10
>  h4   | 16
> (20 rows)
>
> When querying the data, the hash partition uses the same
> algorithm as the insertion, and filters out the table
> that does not need to be scanned.
>
> postgres=# explain analyze select * from h where id = 1;
>  QUERY PLAN
>
> 
> 
>  Append  (cost=0.00..41.88 rows=13 width=4) (actual time=
> 0.020..0.023 rows=1 loops=1)
>->  Seq Scan on h3  (cost=0.00..41.88 rows=13 width=4) (
> actual time=0.013..0.016 rows=1 loops=1)
>  Filter: (id = 1)
>  Rows Removed by Filter: 3
>  Planning time: 0.346 ms
>  Execution time: 0.061 ms
> (6 rows)
>
> postgres=# explain analyze select * from h where id in (1,5);;
>  QUERY PLAN
>
> 
> 
>  Append  (cost=0.00..83.75 rows=52 width=4) (actual time=
> 0.016..0.028 rows=2 loops=1)
>->  Seq Scan on h1  (cost=0.00..41.88 rows=26 width=4) (
> actual time=0.015..0.018 rows=1 loops=1)
>  Filter: (id = ANY ('{1,5}'::integer[]))
>  Rows Removed by Filter: 6
>->  Seq Scan on h3  (cost=0.00..41.88 rows=26 width=4) (
> actual time=0.005..0.007 rows=1 loops=1)
>  Filter: (id = ANY ('{1,5}'::integer[]))
>  Rows Removed by Filter: 3
>  Planning time: 0.720 ms
>  Execution time: 0.074 ms
> (9 rows)
>
> postgres=# explain analyze select * from h where id = 1 or id = 5;;
>  QUERY PLAN
>
> 
> 
>  Append  (cost=0.00..96.50 rows=50 width=4) (actual time=
> 0.017..0.078 rows=2 loops=1)
>->  Seq Scan on h1  (cost=0.00..48.25 rows=25 width=4) (
> actual time=0.015..0.019 rows=1 loops=1)
>  Filter: ((id = 1) OR (id 

Re: [HACKERS] hash partitioning based on v10Beta2

[Rushabh Lathia]

On Sat, Aug 26, 2017 at 10:10 AM, yang...@highgo.com 
wrote:

> Hi all,
>
> Now we have had the range / list partition, but hash partitioning is not
> implemented yet.
> Attached is a POC patch based on the v10Beta2 to add the
> hash partitioning feature.
> Although we will need more discussions about the syntax
> and other specifications before going ahead the project,
> but I think this runnable code might help to discuss
> what and how we implement this.
>
>
FYI, there is already an existing commitfest entry for this project.

https://commitfest.postgresql.org/14/1059/



> Description
>
> The hash partition's implement is on the basis of
> the original range / list partition,and using similar syntax.
>
> To create a partitioned table ,use:
>
> CREATE TABLE h (id int) PARTITION BY HASH(id);
>
> The partitioning key supports only one value, and I think
> the partition key can support multiple values,
> which may be difficult to implement when querying, but
> it is not impossible.
>
> A partition table can be create as bellow:
>
>  CREATE TABLE h1 PARTITION OF h;
>  CREATE TABLE h2 PARTITION OF h;
>  CREATE TABLE h3 PARTITION OF h;
>
> FOR VALUES clause cannot be used, and the partition bound
> is calclulated automatically as partition index of single integer value.
>
> An inserted record is stored in a partition whose index equals
> DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0],
> TYPECACHE_HASH_PROC)->hash_proc, values[0])) % nparts
> /* Number of partitions */
> ;
> In the above example, this is DatumGetUInt32(OidFunctionCall1(lookup_type_
> cache(key->parttypid[0], TYPECACHE_HASH_PROC)->hash_proc, id)) % 3;
>
> postgres=# insert into h select generate_series(1,20);
> INSERT 0 20
> postgres=# select tableoid::regclass,* from h;
>  tableoid | id
> --+
>  h1   |  3
>  h1   |  5
>  h1   | 17
>  h1   | 19
>  h2   |  2
>  h2   |  6
>  h2   |  7
>  h2   | 11
>  h2   | 12
>  h2   | 14
>  h2   | 15
>  h2   | 18
>  h2   | 20
>  h3   |  1
>  h3   |  4
>  h3   |  8
>  h3   |  9
>  h3   | 10
>  h3   | 13
>  h3   | 16
> (20 rows)
>
> The number of partitions here can be dynamically added, and
> if a new partition is created, the number of partitions
> changes, the calculated target partitions will change,
>  and the same data is not reasonable in different
> partitions,So you need to re-calculate the existing data
> and insert the target partition when you create a new partition.
>
> postgres=# create table h4 partition of h;
> CREATE TABLE
> postgres=# select tableoid::regclass,* from h;
>  tableoid | id
> --+
>  h1   |  5
>  h1   | 17
>  h1   | 19
>  h1   |  6
>  h1   | 12
>  h1   |  8
>  h1   | 13
>  h2   | 11
>  h2   | 14
>  h3   |  1
>  h3   |  9
>  h3   |  2
>  h3   | 15
>  h4   |  3
>  h4   |  7
>  h4   | 18
>  h4   | 20
>  h4   |  4
>  h4   | 10
>  h4   | 16
> (20 rows)
>
> When querying the data, the hash partition uses the same
> algorithm as the insertion, and filters out the table
> that does not need to be scanned.
>
> postgres=# explain analyze select * from h where id = 1;
>  QUERY PLAN
>
> 
> 
>  Append  (cost=0.00..41.88 rows=13 width=4) (actual time=
> 0.020..0.023 rows=1 loops=1)
>->  Seq Scan on h3  (cost=0.00..41.88 rows=13 width=4) (
> actual time=0.013..0.016 rows=1 loops=1)
>  Filter: (id = 1)
>  Rows Removed by Filter: 3
>  Planning time: 0.346 ms
>  Execution time: 0.061 ms
> (6 rows)
>
> postgres=# explain analyze select * from h where id in (1,5);;
>  QUERY PLAN
>
> 
> 
>  Append  (cost=0.00..83.75 rows=52 width=4) (actual time=
> 0.016..0.028 rows=2 loops=1)
>->  Seq Scan on h1  (cost=0.00..41.88 rows=26 width=4) (
> actual time=0.015..0.018 rows=1 loops=1)
>  Filter: (id = ANY ('{1,5}'::integer[]))
>  Rows Removed by Filter: 6
>->  Seq Scan on h3  (cost=0.00..41.88 rows=26 width=4) (
> actual time=0.005..0.007 rows=1 loops=1)
>  Filter: (id = ANY ('{1,5}'::integer[]))
>  Rows Removed by Filter: 3
>  Planning time: 0.720 ms
>  Execution time: 0.074 ms
> (9 rows)
>
> postgres=# explain analyze select * from h where id = 1 or id = 5;;
>  QUERY PLAN
>
> 
> 
>  Append  (cost=0.00..96.50 rows=50 width=4) (actual time=
> 0.017..0.078 rows=2 loops=1)
>->  Seq Scan on h1  (cost=0.00..48.25 rows=25 width=4) (
> actual time=0.015..0.019 rows=1 loops=1)
>  Filter: ((id = 1) OR (id 

[HACKERS] hash partitioning based on v10Beta2

[yang...@highgo.com]

Hi all,

Now we have had the range / list partition, but hash partitioning is not 
implemented yet. 
Attached is a POC patch based on the v10Beta2 to add the hash partitioning 
feature. 
Although we will need more discussions about the syntax and other 
specifications before going ahead the project, 
but I think this runnable code might help to discuss what and how we implement 
this.

Description

The hash partition's implement is on the basis of the original range / list 
partition,and using similar syntax.

To create a partitioned table ,use:

CREATE TABLE h (id int) PARTITION BY HASH(id);

The partitioning key supports only one value, and I think the partition key can 
support multiple values, 
which may be difficult to implement when querying, but it is not impossible.

A partition table can be create as bellow:

 CREATE TABLE h1 PARTITION OF h;
 CREATE TABLE h2 PARTITION OF h;
 CREATE TABLE h3 PARTITION OF h;
 
FOR VALUES clause cannot be used, and the partition bound is calclulated 
automatically as partition index of single integer value.

An inserted record is stored in a partition whose index equals 
DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], 
TYPECACHE_HASH_PROC)->hash_proc, values[0])) % nparts /* Number of partitions */
;
In the above example, this is 
DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], 
TYPECACHE_HASH_PROC)->hash_proc, id)) % 3;

postgres=# insert into h select generate_series(1,20);
INSERT 0 20
postgres=# select tableoid::regclass,* from h;
 tableoid | id 
--+
 h1   |  3
 h1   |  5
 h1   | 17
 h1   | 19
 h2   |  2
 h2   |  6
 h2   |  7
 h2   | 11
 h2   | 12
 h2   | 14
 h2   | 15
 h2   | 18
 h2   | 20
 h3   |  1
 h3   |  4
 h3   |  8
 h3   |  9
 h3   | 10
 h3   | 13
 h3   | 16
(20 rows)

The number of partitions here can be dynamically added, and if a new partition 
is created, the number of partitions changes, the calculated target partitions 
will change, and the same data is not reasonable in different partitions,So you 
need to re-calculate the existing data and insert the target partition when you 
create a new partition.

postgres=# create table h4 partition of h;
CREATE TABLE
postgres=# select tableoid::regclass,* from h;
 tableoid | id 
--+
 h1   |  5
 h1   | 17
 h1   | 19
 h1   |  6
 h1   | 12
 h1   |  8
 h1   | 13
 h2   | 11
 h2   | 14
 h3   |  1
 h3   |  9
 h3   |  2
 h3   | 15
 h4   |  3
 h4   |  7
 h4   | 18
 h4   | 20
 h4   |  4
 h4   | 10
 h4   | 16
(20 rows)

When querying the data, the hash partition uses the same algorithm as the 
insertion, and filters out the table that does not need to be scanned.

postgres=# explain analyze select * from h where id = 1;
 QUERY PLAN 


 Append  (cost=0.00..41.88 rows=13 width=4) (actual time=0.020..0.023 rows=1 
loops=1)
   ->  Seq Scan on h3  (cost=0.00..41.88 rows=13 width=4) (actual 
time=0.013..0.016 rows=1 loops=1)
 Filter: (id = 1)
 Rows Removed by Filter: 3
 Planning time: 0.346 ms
 Execution time: 0.061 ms
(6 rows)

postgres=# explain analyze select * from h where id in (1,5);;
 QUERY PLAN 


 Append  (cost=0.00..83.75 rows=52 width=4) (actual time=0.016..0.028 rows=2 
loops=1)
   ->  Seq Scan on h1  (cost=0.00..41.88 rows=26 width=4) (actual 
time=0.015..0.018 rows=1 loops=1)
 Filter: (id = ANY ('{1,5}'::integer[]))
 Rows Removed by Filter: 6
   ->  Seq Scan on h3  (cost=0.00..41.88 rows=26 width=4) (actual 
time=0.005..0.007 rows=1 loops=1)
 Filter: (id = ANY ('{1,5}'::integer[]))
 Rows Removed by Filter: 3
 Planning time: 0.720 ms
 Execution time: 0.074 ms
(9 rows)

postgres=# explain analyze select * from h where id = 1 or id = 5;;
 QUERY PLAN 


 Append  (cost=0.00..96.50 rows=50 width=4) (actual time=0.017..0.078 rows=2 
loops=1)
   ->  Seq Scan on h1  (cost=0.00..48.25 rows=25 width=4) (actual 
time=0.015..0.019 rows=1 loops=1)
 Filter: ((id = 1) OR (id = 5))
 Rows Removed by Filter: 6
   ->  Seq Scan on h3  (cost=0.00..48.25 rows=25 width=4) (actual 
time=0.005..0.010 rows=1 loops=1)
 Filter: ((id = 1) OR (id = 5))
 Rows Removed by Filter: 3
 Planning time: 0.396 ms
 Execution time: 0.139 ms
(9 rows)

Can not detach / attach / drop partition table.

Re: [HACKERS] New partitioning - some feedback

[Robert Haas]

On Tue, Jul 18, 2017 at 2:26 AM, Vik Fearing
 wrote:
> On 07/07/2017 02:02 AM, Mark Kirkwood wrote:
>> I'd prefer *not* to see a table and its partitions all intermixed in the
>> same display (especially with nothing indicating which are partitions) -
>> as this will make for unwieldy long lists when tables have many
>> partitions. Also it would be good if the 'main' partitioned table and
>> its 'partitions' showed up as a different type in some way.
>
> I've just read through this thread, and I'm wondering why we can't just
> have something like  \set SHOW_PARTITIONS true  or something, and that
> would default to false.

We could, and that would have the advantage of letting people set a
default.  On the other hand, if you want to override the default
behavior just once, adding a modifier character is a lot less typing
than issuing \set, retyping your command, and issuing \set again to
change it back.  So I don't know which is better.

My main point is that it's too late to be making changes upon which we
do not have a clear consensus.  I reject the argument that v11 will be
too late to make this change.  Now that we have partitioning, I
believe there will be zillions of things that need to be done to
improve it further; several of those things already have proposed
patches; this can be another one of those things.  If we rush
something in now and it turns out that it isn't well-liked, we may
well end up with one behavior for v<10, another behavior for v=10, and
a third behavior for v>10.  Better to wait and make the change later
when we have a few more data points.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] New partitioning - some feedback

[Vik Fearing]

On 07/07/2017 02:02 AM, Mark Kirkwood wrote:
> I'd prefer *not* to see a table and its partitions all intermixed in the
> same display (especially with nothing indicating which are partitions) -
> as this will make for unwieldy long lists when tables have many
> partitions. Also it would be good if the 'main' partitioned table and
> its 'partitions' showed up as a different type in some way.

I've just read through this thread, and I'm wondering why we can't just
have something like  \set SHOW_PARTITIONS true  or something, and that
would default to false.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] New partitioning - some feedback

[Mark Kirkwood]

On 16/07/17 05:24, David Fetter wrote:


On Fri, Jul 14, 2017 at 09:49:25PM -0500, Robert Haas wrote:

On Mon, Jul 10, 2017 at 5:46 PM, David Fetter  wrote:

With utmost respect, it's less messy than adding '!' to the already
way too random and mysterious syntax of psql's \ commands.  What
should '\det!' mean?  What about '\dT!'?

Since \det lists foreign tables, \det! would list foreign tables even
if they are partitions.  Plain \det would show only the ones that are
not partitions.

\dT! wouldn't be meaningful, since \dT lists data types and data types
can't be partitions.  If you're trying to conjure up a rule that every
\d command must accept the same set of modifiers, a quick
look at the output of \? and a little experimentation will quickly
show you that neither S nor + apply to all command types, so I see no
reason why that would need to be true for a new modifier either.

TBH, I think we should just leave this well enough alone.  We're
post-beta2 now, there's no clear consensus on what to do here, and
there will be very little opportunity for users to give us feedback if
we stick a change into an August beta3 before a September final
release.

I think a new modifier would be too rushed at this stage, but there's
no reason to throw out the progress on \d vs \dt.




+1

And similarly, there seemed to be a reasonably clear push to label the 
'partitions' as such.


regards

Mark


--
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] New partitioning - some feedback

[David Fetter]

On Fri, Jul 14, 2017 at 09:49:25PM -0500, Robert Haas wrote:
> On Mon, Jul 10, 2017 at 5:46 PM, David Fetter  wrote:
> > With utmost respect, it's less messy than adding '!' to the already
> > way too random and mysterious syntax of psql's \ commands.  What
> > should '\det!' mean?  What about '\dT!'?
> 
> Since \det lists foreign tables, \det! would list foreign tables even
> if they are partitions.  Plain \det would show only the ones that are
> not partitions.
> 
> \dT! wouldn't be meaningful, since \dT lists data types and data types
> can't be partitions.  If you're trying to conjure up a rule that every
> \d command must accept the same set of modifiers, a quick
> look at the output of \? and a little experimentation will quickly
> show you that neither S nor + apply to all command types, so I see no
> reason why that would need to be true for a new modifier either.
> 
> TBH, I think we should just leave this well enough alone.  We're
> post-beta2 now, there's no clear consensus on what to do here, and
> there will be very little opportunity for users to give us feedback if
> we stick a change into an August beta3 before a September final
> release.

I think a new modifier would be too rushed at this stage, but there's
no reason to throw out the progress on \d vs \dt.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] New partitioning - some feedback

[Robert Haas]

On Mon, Jul 10, 2017 at 5:46 PM, David Fetter  wrote:
> With utmost respect, it's less messy than adding '!' to the already
> way too random and mysterious syntax of psql's \ commands.  What
> should '\det!' mean?  What about '\dT!'?

Since \det lists foreign tables, \det! would list foreign tables even
if they are partitions.  Plain \det would show only the ones that are
not partitions.

\dT! wouldn't be meaningful, since \dT lists data types and data types
can't be partitions.  If you're trying to conjure up a rule that every
\d command must accept the same set of modifiers, a quick
look at the output of \? and a little experimentation will quickly
show you that neither S nor + apply to all command types, so I see no
reason why that would need to be true for a new modifier either.

TBH, I think we should just leave this well enough alone.  We're
post-beta2 now, there's no clear consensus on what to do here, and
there will be very little opportunity for users to give us feedback if
we stick a change into an August beta3 before a September final
release.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] New partitioning - some feedback

[Amit Langote]

On 2017/07/13 7:23, Dean Rasheed wrote:
> On 12 July 2017 at 15:58, Alvaro Herrera  wrote:
>> Amit Langote wrote:
>>> On 2017/07/11 13:34, Alvaro Herrera wrote:
 However, the "list tables"
 command \dt should definitely IMO not list partitions.
>>>
>>> Do you mean never?  Even if a modifier is specified?  In the patch I
>>> proposed, \d! (or \d+ or \d++, if '!' turns out to be unpopular) will list
>>> partitions, but \d or \dt won't.  That is, partitions are hidden by default.
>>
>> I don't think there is any need for a single list of all partition of
>> all tables -- is there?  I can't think of anything, but then I haven't
>> been exposed very much to this feature yet.  For now, I lean towards "never".
>>
> 
> So just focusing on the listing issue for now...
> 
> I tend to agree with some of the upstream comments that a bare \d
> should list everything, including partitions, because partitions are
> still tables that you might want to do DML or DDL on.
> 
> Also, if you look at what we already have, \d lists all types of
> relations, and then there are 2-letter commands \dE, \di, \dm, \ds,
> \dt and \dv that list just specific kinds of relations, for example
> \dE lists foreign tables, and \dt lists local tables, specifically
> excluding foreign tables.
> 
> So ISTM that the most logical extension of that is:
> 
>   \d - list all relations, including partitions

\d does leave out indexes, but that seems okay.  I think it might be okay
to show partitions after all.  If we do so, do we indicate somehow that
they are partitions of some table?  Maybe an additional column "Partition"
with values "yes" or "no" that occurs right next to the Type column.
Output would look something like below:

\d
 List of relations
 Schema |   Name|   Type| Partition | Owner
+---+---+---+---
 public | foo   | table | no| amit
 public | foo_a_seq | sequence  | no| amit
 public | xyz   | partitioned table | no| amit
 public | xyz1  | table | yes   | amit
 public | xyz2  | table | yes   | amit
 public | xyz3  | partitioned table | yes   | amit
 public | xyz4  | foreign table | yes   | amit
(7 rows)


>   \dt - list only tables that are not foreign tables or partitions
> of other tables

Note that that list will include partitioned tables.

> (that's not quite an exact extension of the existing logic, because of
> course it's partitioned tables that have the different relkind, not
> the partitions, but the above seems like the most useful behaviour)

We allow creating regular tables, partitioned tables, and foreign tables
as partitions.  Being a partition is really independent from the
considerations with which these 2-letter commands are designed, that is,
the second letters map one-to-one with relkinds (again, an exception made
when showing both regular tables and partitioned table with \dt.)

If we establish a rule that each such 2-letter command will only show the
tables of the corresponding relkind that are not partitions, that is, only
those for which relispartition=false will be shown, then we should find an
extension/modifier such that for each command it enables listing
partitions as well.

Perhaps the idea you mentioned at [1] of using letter 'P' for that purpose
could work.  As you described, \dtP or \dPt shows tables (partitioned or
not) including those that are partitions.  Bare \d will mean \dPtvmsE.

> I also agree that there probably isn't much need for a list that
> *only* includes partitions, but if someone comes up with a convincing
> use case, then we could add another 2-letter command for that.

I too can't imagine needing to see only partitions.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAEZATCWcfFtsbKYcVyqUzoOsxkikQjpi_GdjZ_vL6RcX8iLEsg%40mail.gmail.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] New partitioning - some feedback

[Amit Langote]

On 2017/07/12 23:58, Alvaro Herrera wrote:
> Amit Langote wrote:
>> On 2017/07/11 13:34, Alvaro Herrera wrote:
>>> Robert Haas wrote:
 On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
  wrote:
>>>
> Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
> "partitioned table", we wouldn't need a separate flag for marking a table
> as having partitions.

 I think that is false.  Whether something is partitioned and whether
 it is a partition are independent concerns.
>>>
>>> Maybe this discussion is easier if we differentiate "list tables" (\dt,
>>> or \d without a pattern) from "describe table" (\d with a name pattern).
>>
>> I think this discussion has mostly focused on "list tables" so far.
> 
> Yes, which I think is a mistake, because for some things you definitely
> need a list of partitions of the table in question.  And "describe
> table" can fulfill that role perfectly well, ISTM.

For a partitioned table, "describe table" (aka \d name_pattern) lists its
partitions showing for each partition its name and the partition bound.
"list tables/view/indexes/..." (aka \d[tvi...]) shows information about
the listed objects that one might want to see for partitions (such as the
schema, owner, size, description) and "describe table" doesn't provide
that about partitions as just mentioned.  So, it should be possible to
list partitions in some way.

>>> However, the "list tables"
>>> command \dt should definitely IMO not list partitions.
>>
>> Do you mean never?  Even if a modifier is specified?  In the patch I
>> proposed, \d! (or \d+ or \d++, if '!' turns out to be unpopular) will list
>> partitions, but \d or \dt won't.  That is, partitions are hidden by default.
> 
> I don't think there is any need for a single list of all partition of
> all tables -- is there?  I can't think of anything, but then I haven't
> been exposed very much to this feature yet.  For now, I lean towards "never".
> 
> (A different consideration is the use case of listing relation
> relfrozenxid/relminmxid ages, but that use case is already not fulfilled
> by psql metacommands so you still need custom catalog queries).

As I mentioned above, if we decide to hide partitions except when
"describing" the parent table, one would need custom queries even to see
schema, owner, etc. for partitions.

> I don't think \d! works terribly well as a mental model, but maybe
> that's just me.

It seems you're not alone.  Anyway, I'm starting to like Dean's advice [1]
on this matter.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/caezatcwcfftsbkycvyquzoosxkikqjpi_gdjz_vl6rcx8il...@mail.gmail.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] New partitioning - some feedback

[Dean Rasheed]

On 12 July 2017 at 23:23, Dean Rasheed  wrote:
> I also agree that there probably isn't much need for a list that
> *only* includes partitions, but if someone comes up with a convincing
> use case, then we could add another 2-letter command for that.
>

Actually, I just thought of a round-about sort of use case:

The various 2-letter commands \dE, \dt, etc... are designed to work
together, so you can do things like \dEt or \dtE to list all local and
foreign tables, whilst excluding views, sequences, etc. So, if for the
sake of argument, \dP were made to list partitions, then you'd be able
to do things like \dEPt to list all the various kinds of tables,
including partitions, whilst excluding views, etc.

That seems somewhat more elegant and flexible than \d++ or \d! or whatever.

Of course, you'd have to decide whether a foreign partition came under
\dE, \dP, both or something else. I'm not sure that we should eat
another letter of the alphabet just for that case, because there
aren't many left, and I don't think any will be natural mnemonics like
the others.

Regards,
Dean


-- 
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] New partitioning - some feedback

[Dean Rasheed]

On 12 July 2017 at 15:58, Alvaro Herrera  wrote:
> Amit Langote wrote:
>> On 2017/07/11 13:34, Alvaro Herrera wrote:
>> > However, the "list tables"
>> > command \dt should definitely IMO not list partitions.
>>
>> Do you mean never?  Even if a modifier is specified?  In the patch I
>> proposed, \d! (or \d+ or \d++, if '!' turns out to be unpopular) will list
>> partitions, but \d or \dt won't.  That is, partitions are hidden by default.
>
> I don't think there is any need for a single list of all partition of
> all tables -- is there?  I can't think of anything, but then I haven't
> been exposed very much to this feature yet.  For now, I lean towards "never".
>

So just focusing on the listing issue for now...

I tend to agree with some of the upstream comments that a bare \d
should list everything, including partitions, because partitions are
still tables that you might want to do DML or DDL on.

Also, if you look at what we already have, \d lists all types of
relations, and then there are 2-letter commands \dE, \di, \dm, \ds,
\dt and \dv that list just specific kinds of relations, for example
\dE lists foreign tables, and \dt lists local tables, specifically
excluding foreign tables.

So ISTM that the most logical extension of that is:

  \d - list all relations, including partitions

  \dt - list only tables that are not foreign tables or partitions
of other tables

(that's not quite an exact extension of the existing logic, because of
course it's partitioned tables that have the different relkind, not
the partitions, but the above seems like the most useful behaviour)

With this, I don't think there's any need for any additional
modifiers, like ! or ++.

I also agree that there probably isn't much need for a list that
*only* includes partitions, but if someone comes up with a convincing
use case, then we could add another 2-letter command for that.


> I don't think \d! works terribly well as a mental model, but maybe
> that's just me.
>

Yeah, I agree. It just looks ugly somehow.


>> So it seems most of us are in favor for showing partitioned tables as
>> "partitioned table" instead of "table" in the table listing.
>
> Yeah, that sounds good to me.
>

+1

Regards,
Dean


-- 
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] New partitioning - some feedback

[Alvaro Herrera]

Amit Langote wrote:
> On 2017/07/11 13:34, Alvaro Herrera wrote:
> > Robert Haas wrote:
> >> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
> >>  wrote:
> > 
> >>> Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
> >>> "partitioned table", we wouldn't need a separate flag for marking a table
> >>> as having partitions.
> >>
> >> I think that is false.  Whether something is partitioned and whether
> >> it is a partition are independent concerns.
> > 
> > Maybe this discussion is easier if we differentiate "list tables" (\dt,
> > or \d without a pattern) from "describe table" (\d with a name pattern).
> 
> I think this discussion has mostly focused on "list tables" so far.

Yes, which I think is a mistake, because for some things you definitely
need a list of partitions of the table in question.  And "describe
table" can fulfill that role perfectly well, ISTM.

> > It seems to me that the "describe" command should list partitions --
> > perhaps only when the + flag is given.
> 
> That's what happens today.

So no further changes needed there -- good.

> > However, the "list tables"
> > command \dt should definitely IMO not list partitions.
> 
> Do you mean never?  Even if a modifier is specified?  In the patch I
> proposed, \d! (or \d+ or \d++, if '!' turns out to be unpopular) will list
> partitions, but \d or \dt won't.  That is, partitions are hidden by default.

I don't think there is any need for a single list of all partition of
all tables -- is there?  I can't think of anything, but then I haven't
been exposed very much to this feature yet.  For now, I lean towards "never".

(A different consideration is the use case of listing relation
relfrozenxid/relminmxid ages, but that use case is already not fulfilled
by psql metacommands so you still need custom catalog queries).

I don't think \d! works terribly well as a mental model, but maybe
that's just me.

> > Maybe \dt should
> > have some flag indicating whether each table is partitioned.
> 
> So it seems most of us are in favor for showing partitioned tables as
> "partitioned table" instead of "table" in the table listing.

Yeah, that sounds good to me.

-- 
Álvaro Herrerahttps://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] New partitioning - some feedback

[Ashutosh Bapat]

On Wed, Jul 12, 2017 at 9:39 AM, Amit Langote
 wrote:
> On 2017/07/12 12:47, Ashutosh Bapat wrote:
>> On Wed, Jul 12, 2017 at 8:23 AM, Amit Langote
>>  wrote:
>>> On 2017/07/11 18:57, Ashutosh Bapat wrote:
 On Tue, Jul 11, 2017 at 4:16 AM, David Fetter  wrote:
> So whatever we land on needs to mention partition_of and
> has_partitions.  Is that latter just its immediate partitions?
> Recursion all the way down?  Somewhere in between?
>

 We have patches proposed to address some of those concerns at [1]

 [1] 
 https://www.postgresql.org/message-id/CAFjFpRcs5fOSfaAGAjT5C6=yvdd7mrx3knf_spb5dqzojgj...@mail.gmail.com
>>>
>>> ISTM, David is talking about the "list tables" (bare \d without any
>>> pattern) case.  That is, listing partitioned tables as of type
>>> "partitioned table" instead of "table" as we currently do.  The linked
>>> patch, OTOH, is for "describe table" (\d ) case.
>>
>> Right, the patches don't exactly do what David is suggesting, but
>> those I believe have code to annotate the tables with "has partitions"
>> and also the number of partitions (I guess). Although, that thread has
>> died some time ago, so my memory can be vague.
>>
>> Do you see that those patches can be used in current discussion in any way?
>
> It wouldn't really be a bad idea to put that patch here, because there's
> no special reason for it to be in the CF for PG 11, if we are talking here
> about changing \d command outputs anyway.

Thanks.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
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] New partitioning - some feedback

[Amit Langote]

On 2017/07/12 13:09, Amit Langote wrote:
> On 2017/07/12 12:47, Ashutosh Bapat wrote:
>> Do you see that those patches can be used in current discussion in any way?
> 
> It wouldn't really be a bad idea to put that patch here, because there's
> no special reason for it to be in the CF for PG 11, if we are talking here
> about changing \d command outputs anyway.

So, here are 4 patches (including the 2 patches that Ashutosh linked to
upthread):

0001: Show relispartition=true relations as "(foreign) partition" and
  RELKIND_PARTITIONED_TABLE relations that are not themselves
  partitions as "partitioned table"

0002: Hide relispartition=true relations (partitions) by default in the
  \d listing (that is, \d without a name pattern); to enable
  displaying partitions, add a modifier '++'

0003: In \d+ partitioned_table output (describe partitioned table showing
  individual partitions), show if the individual partitions are
  partitioned themselves if it actually does have partitions
  currently

0004: In \d+ partitioned_table output, do not skip the portion of the
  output showing information about partitions if there are currently
  no partitions defined; instead show "Number of partitions: 0"


Regarding 0001, while it shows "partition" and "partitioned table" in the
Type column of \d listing, \d name_pattern will still show Table
"schemaname.tablename".  For example:

\d
 List of relations
 Schema | Name  |   Type| Owner
+---+---+---
 public | xyz   | partitioned table | amit
 public | xyz1  | partition | amit
 public | xyz2  | partition | amit
 public | xyz3  | partition | amit
 public | xyz31 | partition | amit
(5 rows)

\d xyz*
Table "public.xyz"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   |  |
Partition key: LIST (a)
Number of partitions: 3 (Use \d+ to list them.)

Table "public.xyz1"

Table "public.xyz2"

Table "public.xyz3"

Table "public.xyz31"


...which might seem kind of odd.  Do we want to show xyz1 as "Partition
public.xyz1", for example?

Thanks,
Amit
From ceacc566ab7ac2ffe56a47435a53a12ebafdffe5 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 10 Jul 2017 13:25:20 +0900
Subject: [PATCH 1/4] Show partitions and partitioned tables as such in \d
 listing

---
 src/bin/psql/describe.c | 51 -
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e6833eced5..4613490f56 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3321,27 +3321,60 @@ listTables(const char *tabtypes, const char *pattern, 
bool verbose, bool showSys
printfPQExpBuffer(,
  "SELECT n.nspname as \"%s\",\n"
  "  c.relname as \"%s\",\n"
- "  CASE c.relkind"
- " WHEN " 
CppAsString2(RELKIND_RELATION) " THEN '%s'"
+ "  CASE c.relkind",
+ gettext_noop("Schema"),
+ gettext_noop("Name"));
+
+   /*
+* Starting in PG 10, certain kinds of relations could be partitions, 
which
+* if so, we show Type accordingly.
+*/
+   if (pset.sversion >= 10)
+   appendPQExpBuffer(,
+ " WHEN " 
CppAsString2(RELKIND_RELATION) " THEN"
+ "   CASE c.relispartition"
+ " WHEN 'true' THEN '%s' 
ELSE '%s'"
+ "   END"
+
+ " WHEN " 
CppAsString2(RELKIND_PARTITIONED_TABLE) " THEN"
+ "   CASE c.relispartition"
+ " WHEN 'true' THEN '%s' 
ELSE '%s'"
+ "   END"
+
+ " WHEN " 
CppAsString2(RELKIND_FOREIGN_TABLE) " THEN"
+ "   CASE c.relispartition"
+ " WHEN 'true' THEN '%s' 
ELSE '%s'"
+ "   END",
+ gettext_noop("partition"),
+ gettext_noop("table"),
+
+ gettext_noop("partition"),
+ gettext_noop("partitioned 
table"),
+
+

Re: [HACKERS] New partitioning - some feedback

[Amit Langote]

On 2017/07/12 12:47, Ashutosh Bapat wrote:
> On Wed, Jul 12, 2017 at 8:23 AM, Amit Langote
>  wrote:
>> On 2017/07/11 18:57, Ashutosh Bapat wrote:
>>> On Tue, Jul 11, 2017 at 4:16 AM, David Fetter  wrote:
 So whatever we land on needs to mention partition_of and
 has_partitions.  Is that latter just its immediate partitions?
 Recursion all the way down?  Somewhere in between?

>>>
>>> We have patches proposed to address some of those concerns at [1]
>>>
>>> [1] 
>>> https://www.postgresql.org/message-id/CAFjFpRcs5fOSfaAGAjT5C6=yvdd7mrx3knf_spb5dqzojgj...@mail.gmail.com
>>
>> ISTM, David is talking about the "list tables" (bare \d without any
>> pattern) case.  That is, listing partitioned tables as of type
>> "partitioned table" instead of "table" as we currently do.  The linked
>> patch, OTOH, is for "describe table" (\d ) case.
> 
> Right, the patches don't exactly do what David is suggesting, but
> those I believe have code to annotate the tables with "has partitions"
> and also the number of partitions (I guess). Although, that thread has
> died some time ago, so my memory can be vague.
> 
> Do you see that those patches can be used in current discussion in any way?

It wouldn't really be a bad idea to put that patch here, because there's
no special reason for it to be in the CF for PG 11, if we are talking here
about changing \d command outputs anyway.

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] New partitioning - some feedback

[Ashutosh Bapat]

On Wed, Jul 12, 2017 at 8:23 AM, Amit Langote
 wrote:
> On 2017/07/11 18:57, Ashutosh Bapat wrote:
>> On Tue, Jul 11, 2017 at 4:16 AM, David Fetter  wrote:
>>> So whatever we land on needs to mention partition_of and
>>> has_partitions.  Is that latter just its immediate partitions?
>>> Recursion all the way down?  Somewhere in between?
>>>
>>
>> We have patches proposed to address some of those concerns at [1]
>>
>> [1] 
>> https://www.postgresql.org/message-id/CAFjFpRcs5fOSfaAGAjT5C6=yvdd7mrx3knf_spb5dqzojgj...@mail.gmail.com
>
> ISTM, David is talking about the "list tables" (bare \d without any
> pattern) case.  That is, listing partitioned tables as of type
> "partitioned table" instead of "table" as we currently do.  The linked
> patch, OTOH, is for "describe table" (\d ) case.

Right, the patches don't exactly do what David is suggesting, but
those I believe have code to annotate the tables with "has partitions"
and also the number of partitions (I guess). Although, that thread has
died some time ago, so my memory can be vague.

Do you see that those patches can be used in current discussion in any way?


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
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] New partitioning - some feedback

[Amit Langote]

On 2017/07/11 13:34, Alvaro Herrera wrote:
> Robert Haas wrote:
>> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
>>  wrote:
> 
>>> Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
>>> "partitioned table", we wouldn't need a separate flag for marking a table
>>> as having partitions.
>>
>> I think that is false.  Whether something is partitioned and whether
>> it is a partition are independent concerns.
> 
> Maybe this discussion is easier if we differentiate "list tables" (\dt,
> or \d without a pattern) from "describe table" (\d with a name pattern).

I think this discussion has mostly focused on "list tables" so far.

> It seems to me that the "describe" command should list partitions --
> perhaps only when the + flag is given.

That's what happens today.

> However, the "list tables"
> command \dt should definitely IMO not list partitions.

Do you mean never?  Even if a modifier is specified?  In the patch I
proposed, \d! (or \d+ or \d++, if '!' turns out to be unpopular) will list
partitions, but \d or \dt won't.  That is, partitions are hidden by default.

> Maybe \dt should
> have some flag indicating whether each table is partitioned.

So it seems most of us are in favor for showing partitioned tables as
"partitioned table" instead of "table" in the table listing.

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] New partitioning - some feedback

[Amit Langote]

On 2017/07/11 18:57, Ashutosh Bapat wrote:
> On Tue, Jul 11, 2017 at 4:16 AM, David Fetter  wrote:
>> So whatever we land on needs to mention partition_of and
>> has_partitions.  Is that latter just its immediate partitions?
>> Recursion all the way down?  Somewhere in between?
>>
> 
> We have patches proposed to address some of those concerns at [1]
> 
> [1] 
> https://www.postgresql.org/message-id/CAFjFpRcs5fOSfaAGAjT5C6=yvdd7mrx3knf_spb5dqzojgj...@mail.gmail.com

ISTM, David is talking about the "list tables" (bare \d without any
pattern) case.  That is, listing partitioned tables as of type
"partitioned table" instead of "table" as we currently do.  The linked
patch, OTOH, is for "describe table" (\d ) case.

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] New partitioning - some feedback

[Ashutosh Bapat]

On Tue, Jul 11, 2017 at 4:16 AM, David Fetter  wrote:
> On Mon, Jul 10, 2017 at 05:33:34PM -0500, Robert Haas wrote:
>> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
>>  wrote:
>> > I posted a patch upthread which makes \d hide partitions
>> > (relispartition = true relations) and include them if the newly
>> > proposed '!' modifier is specified.  The '+' modifier is being
>> > used to show additional detail of relations chosen to be listed at
>> > all, so it seemed like a bad idea to extend its meaning to also
>> > dictate whether partitions are to be listed.
>>
>> +1.  That'd be a mess.
>
> With utmost respect, it's less messy than adding '!' to the already
> way too random and mysterious syntax of psql's \ commands.  What
> should '\det!' mean?  What about '\dT!'?
>
>> > Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of
>> > Type "partitioned table", we wouldn't need a separate flag for
>> > marking a table as having partitions.
>>
>> I think that is false.  Whether something is partitioned and whether
>> it is a partition are independent concerns.
>
> So whatever we land on needs to mention partition_of and
> has_partitions.  Is that latter just its immediate partitions?
> Recursion all the way down?  Somewhere in between?
>

We have patches proposed to address some of those concerns at [1]

[1] 
https://www.postgresql.org/message-id/CAFjFpRcs5fOSfaAGAjT5C6=yvdd7mrx3knf_spb5dqzojgj...@mail.gmail.com

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
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] New partitioning - some feedback

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
>  wrote:

> > Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
> > "partitioned table", we wouldn't need a separate flag for marking a table
> > as having partitions.
> 
> I think that is false.  Whether something is partitioned and whether
> it is a partition are independent concerns.

Maybe this discussion is easier if we differentiate "list tables" (\dt,
or \d without a pattern) from "describe table" (\d with a name pattern).

It seems to me that the "describe" command should list partitions --
perhaps only when the + flag is given.  However, the "list tables"
command \dt should definitely IMO not list partitions.  Maybe \dt should
have some flag indicating whether each table is partitioned.


-- 
Álvaro Herrerahttps://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] New partitioning - some feedback

[Amit Langote]

On 2017/07/11 10:34, Paul A Jungwirth wrote:
>> Also, there seems to be at least some preference
>> for excluding partitions by default from the \d listing.
> 
> As another user of partitions I'll chime in and say that would be very
> nice! On the other hand, with pre-10 partitions you do see all the
> child tables with `\d`, so showing declarative partitions seems more
> consistent with the old functionality.

That's one way of looking at it. :)

> On the third hand with pre-10 partitions I can put the child tables
> into a separate schema to de-clutter `\d`, but I don't see any way to
> do that with declarative partitions. Since there is no workaround it
> makes it a bit more important for partitions not to be so noisy.

You can do that with declarative partitions:

create table foo (a int) partition by list (a);

create schema foo_parts;
create table foo_parts.foo1 partition of foo for values in (1);
create table foo_parts.foo2 partition of foo for values in (2);

\d
   List of relations
 Schema | Name | Type  | Owner
+--+---+---
 public | foo  | table | amit
(1 row)

set search_path to foo_parts;

\d
List of relations
  Schema   | Name | Type  | Owner
---+--+---+---
 foo_parts | foo1 | table | amit
 foo_parts | foo2 | table | amit
(2 rows)

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] New partitioning - some feedback

[Paul A Jungwirth]

> Also, there seems to be at least some preference
> for excluding partitions by default from the \d listing.

As another user of partitions I'll chime in and say that would be very
nice! On the other hand, with pre-10 partitions you do see all the
child tables with `\d`, so showing declarative partitions seems more
consistent with the old functionality.

On the third hand with pre-10 partitions I can put the child tables
into a separate schema to de-clutter `\d`, but I don't see any way to
do that with declarative partitions. Since there is no workaround it
makes it a bit more important for partitions not to be so noisy.

Paul


-- 
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] New partitioning - some feedback

[Amit Langote]

On 2017/07/11 7:33, Robert Haas wrote:
> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote  
> wrote:
>> Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
>> "partitioned table", we wouldn't need a separate flag for marking a table
>> as having partitions.
> 
> I think that is false.  Whether something is partitioned and whether
> it is a partition are independent concerns.

I meant to speak of RELKIND_PARTITIONED_TABLE tables as having partitions
(although it could be a partition itself too).  If based on the relkind,
we had shown their type as "partitioned table" (not just "table"), then we
wouldn't need a separate flag/column in the \d output to distinguish
partitioned tables as being different from regular tables, as Craig seemed
to be proposing.

Since we are going the route of showing relispartition = true relations as
of different type in the \d listing (as "partition"/"foreign partition"),
we might as well go and spell RELKIND_PARTITIONED_TABLE tables as
"partitioned table".  But, I'm afraid that it would be a much bigger
change if we don't want to restrict this terminology change to \d listing;
error messages don't bother about distinguishing "partitions"
(relispartition = true) or "partitioned tables"
(RELKIND_PARTITIONED_TABLE), for instance.

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] New partitioning - some feedback

[Greg Stark]

On 10 July 2017 at 23:46, David Fetter  wrote:
> On Mon, Jul 10, 2017 at 05:33:34PM -0500, Robert Haas wrote:
>> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
>>  wrote:
>> > I posted a patch upthread which makes \d hide partitions
>> > (relispartition = true relations) and include them if the newly
>> > proposed '!' modifier is specified.  The '+' modifier is being
>> > used to show additional detail of relations chosen to be listed at
>> > all, so it seemed like a bad idea to extend its meaning to also
>> > dictate whether partitions are to be listed.
>>
>> +1.  That'd be a mess.
>
> With utmost respect, it's less messy than adding '!' to the already
> way too random and mysterious syntax of psql's \ commands.  What
> should '\det!' mean?  What about '\dT!'?

Fwiw as, I believe, the first person to make this complaint I would be
fine with + listing all partitions. Imho adding an orthogonal "!"
would be too much mental overhead for the user.

If you want something different perhaps we can invent ++ for "even
more information" and list partitions only if two plusses are
provided. (I don't think the other way around makes sense since you
might need a way to list permissions and comments without listing
every partition if you're on a system with an unmanageable number of
partitions but you never absolutely need a way to list partitions
without the comments and permissions). At least that doesn't require
the user to learn a new flag and how it interacts with everything
else.

-- 
greg


-- 
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] New partitioning - some feedback

[David Fetter]

On Mon, Jul 10, 2017 at 05:33:34PM -0500, Robert Haas wrote:
> On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
>  wrote:
> > I posted a patch upthread which makes \d hide partitions
> > (relispartition = true relations) and include them if the newly
> > proposed '!' modifier is specified.  The '+' modifier is being
> > used to show additional detail of relations chosen to be listed at
> > all, so it seemed like a bad idea to extend its meaning to also
> > dictate whether partitions are to be listed.
> 
> +1.  That'd be a mess.

With utmost respect, it's less messy than adding '!' to the already
way too random and mysterious syntax of psql's \ commands.  What
should '\det!' mean?  What about '\dT!'?

> > Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of
> > Type "partitioned table", we wouldn't need a separate flag for
> > marking a table as having partitions.
> 
> I think that is false.  Whether something is partitioned and whether
> it is a partition are independent concerns.

So whatever we land on needs to mention partition_of and
has_partitions.  Is that latter just its immediate partitions?
Recursion all the way down?  Somewhere in between?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] New partitioning - some feedback

[Robert Haas]

On Mon, Jul 10, 2017 at 2:15 AM, Amit Langote
 wrote:
> I posted a patch upthread which makes \d hide partitions (relispartition =
> true relations) and include them if the newly proposed '!' modifier is
> specified.  The '+' modifier is being used to show additional detail of
> relations chosen to be listed at all, so it seemed like a bad idea to
> extend its meaning to also dictate whether partitions are to be listed.

+1.  That'd be a mess.

> Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
> "partitioned table", we wouldn't need a separate flag for marking a table
> as having partitions.

I think that is false.  Whether something is partitioned and whether
it is a partition are independent concerns.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] New partitioning - some feedback

[David Fetter]

On Mon, Jul 10, 2017 at 04:15:28PM +0900, Amit Langote wrote:
> On 2017/07/10 15:32, Craig Ringer wrote:
> > On 8 July 2017 at 00:03, David Fetter  wrote:
> > 
> >> On Fri, Jul 07, 2017 at 10:29:26AM +0900, Amit Langote wrote:
> >>> Hi Mark,
> >>>
> >>> On 2017/07/07 9:02, Mark Kirkwood wrote:
>  I've been trying out the new partitioning in version 10. Firstly, I
> >> must
>  say this is excellent - so much nicer than the old inheritance based
> >> method!
> >>>
> >>> Thanks. :)
> >>>
>  My only niggle is the display of partitioned tables via \d etc. e.g:
> 
>  part=# \d
>  List of relations
>   Schema | Name | Type  |  Owner
>  +--+---+--
>   public | date_fact| table | postgres
>   public | date_fact_201705 | table | postgres
>   public | date_fact_201706 | table | postgres
>   public | date_fact_20170601   | table | postgres
>   public | date_fact_2017060100 | table | postgres
>   public | date_fact_201707 | table | postgres
>   public | date_fact_rest   | table | postgres
>  (7 rows)
> >>
> >> Would showing relispartition=tru tables only in \d+ fix this?
> >> 
> >>
> > 
> > I think so.
> 
> I posted a patch upthread which makes \d hide partitions (relispartition =
> true relations) and include them if the newly proposed '!' modifier is
> specified.  The '+' modifier is being used to show additional detail of
> relations chosen to be listed at all, so it seemed like a bad idea to
> extend its meaning to also dictate whether partitions are to be listed.
> We have a separate 'S' modifier to ask to list system objects (which are,
> by default hidden), so it made sense to me to add yet another modifier
> (aforementioned '!') for partitions.

We have already made '+' signal "more detail, unspecified," for a lot
of different cases.  If partitions are just "more detail" about a
table, which is the direction we've decided to go, it makes sense to
list them under the rubric of '+' rather than inventing yet another
hunk of syntax to psql's already confusing \ commands.

> > I'd like to add a flag of some kind to \d column output that marks a table
> > as having partitions, but I can't think of anything narrow enough and still
> > useful.
> 
> Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
> "partitioned table", we wouldn't need a separate flag for marking a table
> as having partitions.  But we've avoided using that term ("partitioned
> table") in the error messages and such, so wouldn't perhaps be a good idea
> to do that here.  But I wonder if we (also) want to distinguish
> partitioned tables from regular tables?  I understood that there is some
> desire for partitions be distinguished when they are listed in the output,
> either by default or by using a modifier.

+1 for showing that they're a different beast.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] New partitioning - some feedback

[Amit Langote]

On 2017/07/10 15:32, Craig Ringer wrote:
> On 8 July 2017 at 00:03, David Fetter  wrote:
> 
>> On Fri, Jul 07, 2017 at 10:29:26AM +0900, Amit Langote wrote:
>>> Hi Mark,
>>>
>>> On 2017/07/07 9:02, Mark Kirkwood wrote:
 I've been trying out the new partitioning in version 10. Firstly, I
>> must
 say this is excellent - so much nicer than the old inheritance based
>> method!
>>>
>>> Thanks. :)
>>>
 My only niggle is the display of partitioned tables via \d etc. e.g:

 part=# \d
 List of relations
  Schema | Name | Type  |  Owner
 +--+---+--
  public | date_fact| table | postgres
  public | date_fact_201705 | table | postgres
  public | date_fact_201706 | table | postgres
  public | date_fact_20170601   | table | postgres
  public | date_fact_2017060100 | table | postgres
  public | date_fact_201707 | table | postgres
  public | date_fact_rest   | table | postgres
 (7 rows)
>>
>> Would showing relispartition=tru tables only in \d+ fix this?
>> 
>>
> 
> I think so.

I posted a patch upthread which makes \d hide partitions (relispartition =
true relations) and include them if the newly proposed '!' modifier is
specified.  The '+' modifier is being used to show additional detail of
relations chosen to be listed at all, so it seemed like a bad idea to
extend its meaning to also dictate whether partitions are to be listed.
We have a separate 'S' modifier to ask to list system objects (which are,
by default hidden), so it made sense to me to add yet another modifier
(aforementioned '!') for partitions.

> I'd like to add a flag of some kind to \d column output that marks a table
> as having partitions, but I can't think of anything narrow enough and still
> useful.

Actually, if \d had shown RELKIND_PARTITIONED_TABLE tables as of Type
"partitioned table", we wouldn't need a separate flag for marking a table
as having partitions.  But we've avoided using that term ("partitioned
table") in the error messages and such, so wouldn't perhaps be a good idea
to do that here.  But I wonder if we (also) want to distinguish
partitioned tables from regular tables?  I understood that there is some
desire for partitions be distinguished when they are listed in the output,
either by default or by using a modifier.

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] New partitioning - some feedback

[Craig Ringer]

On 8 July 2017 at 00:03, David Fetter  wrote:

> On Fri, Jul 07, 2017 at 10:29:26AM +0900, Amit Langote wrote:
> > Hi Mark,
> >
> > On 2017/07/07 9:02, Mark Kirkwood wrote:
> > > I've been trying out the new partitioning in version 10. Firstly, I
> must
> > > say this is excellent - so much nicer than the old inheritance based
> method!
> >
> > Thanks. :)
> >
> > > My only niggle is the display of partitioned tables via \d etc. e.g:
> > >
> > > part=# \d
> > > List of relations
> > >  Schema | Name | Type  |  Owner
> > > +--+---+--
> > >  public | date_fact| table | postgres
> > >  public | date_fact_201705 | table | postgres
> > >  public | date_fact_201706 | table | postgres
> > >  public | date_fact_20170601   | table | postgres
> > >  public | date_fact_2017060100 | table | postgres
> > >  public | date_fact_201707 | table | postgres
> > >  public | date_fact_rest   | table | postgres
> > > (7 rows)
>
> Would showing relispartition=tru tables only in \d+ fix this?
> 
>

I think so.

I'd like to add a flag of some kind to \d column output that marks a table
as having partitions, but I can't think of anything narrow enough and still
useful.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] New partitioning - some feedback

[Amit Langote]

On 2017/07/08 14:12, Mark Kirkwood wrote:
> On 07/07/17 19:54, Michael Banck wrote:
>> On Fri, Jul 07, 2017 at 07:40:55PM +1200, Mark Kirkwood wrote:
>>> On 07/07/17 13:29, Amit Langote wrote:
 Someone complained about this awhile back [1].  And then it came up again
 [2], where Noah appeared to take a stance that partitions should be
 visible in views / output of commands that list "tables".

 Although I too tend to prefer not filling up the \d output space by
 listing partitions (pg_class.relispartition = true relations), there
 wasn't perhaps enough push for creating a patch for that.  If some
 committer is willing to consider such a patch, I can make one.
>>>
>>> Yeah, me too (clearly). However if the consensus is that all these
>>> partition
>>> tables *must* be shown in \d output, then I'd be happy if they were
>>> identified as such rather than just 'table' (e.g 'partition table').
>> +1.
>>
>> Or maybe just 'partition' is enough if 'partition table' would widen the
>> column output unnecessarily.
> 
> Yeah, that is probably better (and 'partition table' is potentially
> confusing as Robert pointed out).

So, it seems at least that showing "partition" as the Type of tables that
are actually partitions is preferable.  I created a patch (attached 0001)
that implements that.  It makes \d show any relations that have
relispartition = true as of type "partition" or "foreign partition".  With
the patch:

create table p (a int) partition by list (a);

-- regular table as partition
create table p1 partition of p for values in (1)

-- foreign table as partition
create foreign data wrapper dummy;
create server dummy foreign data wrapper dummy;
create foreign table p2 partition of p for values in (2) server dummy;

-- partitioned table as partition
create table p3 partition of p for values in (3) partition by list (a);

\d
 List of relations
 Schema | Name |   Type| Owner
+--+---+---
 public | p| table | amit
 public | p1   | partition | amit
 public | p2   | foreign partition | amit
 public | p3   | partition | amit
(4 rows)

Also, there seems to be at least some preference for excluding partitions
by default from the \d listing.  Attached 0002 implements that.  To enable
showing partitions, the patch adds a new modifier '!' to \d (picked '!'
from Robert's email on this thread [1]).  With the patch:

\d
   List of relations
 Schema | Name | Type  | Owner
+--+---+---
 public | p| table | amit
(1 row)

\d!
 List of relations
 Schema | Name |   Type| Owner
+--+---+---
 public | p| table | amit
 public | p1   | partition | amit
 public | p2   | foreign partition | amit
 public | p3   | partition | amit
(4 rows)

\d!+
 List of relations
 Schema | Name |   Type| Owner |  Size   | Description
+--+---+---+-+-
 public | p| table | amit  | 0 bytes |
 public | p1   | partition | amit  | 0 bytes |
 public | p2   | foreign partition | amit  | 0 bytes |
 public | p3   | partition | amit  | 0 bytes |
(4 rows)

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoYNPHFjY%2BObFF9%3DTbX%2BT6ez1FAU%2BsmGuXeoiOMasDc-0g%40mail.gmail.com
From c73da2fcfc81ffa351f96be000ae5d262d828ae1 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 10 Jul 2017 13:25:20 +0900
Subject: [PATCH 1/2] Show "(foreign) partition" as Type in \d output

---
 src/bin/psql/describe.c | 48 +++-
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e6833eced5..bbdac8d50d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3321,27 +3321,57 @@ listTables(const char *tabtypes, const char *pattern, 
bool verbose, bool showSys
printfPQExpBuffer(,
  "SELECT n.nspname as \"%s\",\n"
  "  c.relname as \"%s\",\n"
- "  CASE c.relkind"
- " WHEN " 
CppAsString2(RELKIND_RELATION) " THEN '%s'"
+ "  CASE c.relkind",
+ gettext_noop("Schema"),
+ gettext_noop("Name"));
+
+   /*
+* Starting in PG 10, certain kinds of relations could be partitions, 
which
+* if so, we show Type accordingly.
+*/
+   if (pset.sversion >= 10)
+   appendPQExpBuffer(,
+ " WHEN " 
CppAsString2(RELKIND_RELATION) " THEN"
+ "   CASE c.relispartition"
+  

Re: [HACKERS] New partitioning - some feedback

[Mark Kirkwood]

On 07/07/17 19:54, Michael Banck wrote:


On Fri, Jul 07, 2017 at 07:40:55PM +1200, Mark Kirkwood wrote:

On 07/07/17 13:29, Amit Langote wrote:

Someone complained about this awhile back [1].  And then it came up again
[2], where Noah appeared to take a stance that partitions should be
visible in views / output of commands that list "tables".

Although I too tend to prefer not filling up the \d output space by
listing partitions (pg_class.relispartition = true relations), there
wasn't perhaps enough push for creating a patch for that.  If some
committer is willing to consider such a patch, I can make one.

Yeah, me too (clearly). However if the consensus is that all these partition
tables *must* be shown in \d output, then I'd be happy if they were
identified as such rather than just 'table' (e.g 'partition table').

+1.

Or maybe just 'partition' is enough if 'partition table' would widen the
column output unnecessarily.




Yeah, that is probably better (and 'partition table' is potentially 
confusing as Robert pointed out).


Cheers

Mark



--
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] New partitioning - some feedback

[David Fetter]

On Fri, Jul 07, 2017 at 10:29:26AM +0900, Amit Langote wrote:
> Hi Mark,
> 
> On 2017/07/07 9:02, Mark Kirkwood wrote:
> > I've been trying out the new partitioning in version 10. Firstly, I must
> > say this is excellent - so much nicer than the old inheritance based method!
> 
> Thanks. :)
> 
> > My only niggle is the display of partitioned tables via \d etc. e.g:
> >
> > part=# \d
> > List of relations
> >  Schema | Name | Type  |  Owner
> > +--+---+--
> >  public | date_fact| table | postgres
> >  public | date_fact_201705 | table | postgres
> >  public | date_fact_201706 | table | postgres
> >  public | date_fact_20170601   | table | postgres
> >  public | date_fact_2017060100 | table | postgres
> >  public | date_fact_201707 | table | postgres
> >  public | date_fact_rest   | table | postgres
> > (7 rows)

Would showing relispartition=tru tables only in \d+ fix this?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] New partitioning - some feedback

[Robert Haas]

On Fri, Jul 7, 2017 at 8:20 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I don't have a strong view on whether partitions should be hidden by
>> default, although I lean slightly against it (say, -0.25).  But if we
>> do decide to hide them by default, then I definitely want an
>> easy-to-use modifier that overrides that behavior, like being able to
>> type \d! or whatever to have them included after all.
>
> AIUI the user is responsible for DDL on partitions, like say creating
> indexes for them?  Seems like hiding them is a bad idea given that.
> Also, we need to be careful about calling them something very separate
> from "table", because that would rouse the need to have duplicate syntax
> for every sort of ALTER TABLE and suchlike command that we want to have
> be usable with partitions.  I think we've largely gone the wrong direction
> in that respect with respect to foreign tables and matviews.

Well, I'm not sure what other direction we could have taken there, and
I don't think it follows that just because it's labeled differently in
\d output it has to have different SQL syntax.

On the core question of whether they should be hidden, I think the
answer is that it depends on the situation.  As Simon says, if people
use partitioning with large numbers of partitions, listing many
nearly-identical partition names clutters up the list to an extent
that makes life quite difficult; I've encountered this as a real
usability problem on actual systems.  On the other hand, people with
more modest numbers of partitions (say, 10) might well find it more
convenient to see those names included, because they're legitimate
targets for commands like COMMENT and DROP TABLE and lots of other
things, and somebody might very well find it convenient to be able to
get that with \d rather than \d+ parent_table_name.

As I say, I don't feel hugely strongly about the default behavior, but
I do feel strongly that the idea that only one of the two proposed
behavior is useful is entirely incorrect.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] New partitioning - some feedback

[Simon Riggs]

On 7 July 2017 at 13:20, Tom Lane  wrote:
> Robert Haas  writes:
>> I don't have a strong view on whether partitions should be hidden by
>> default, although I lean slightly against it (say, -0.25).  But if we
>> do decide to hide them by default, then I definitely want an
>> easy-to-use modifier that overrides that behavior, like being able to
>> type \d! or whatever to have them included after all.
>
> AIUI the user is responsible for DDL on partitions, like say creating
> indexes for them?  Seems like hiding them is a bad idea given that.
> Also, we need to be careful about calling them something very separate
> from "table", because that would rouse the need to have duplicate syntax
> for every sort of ALTER TABLE and suchlike command that we want to have
> be usable with partitions.  I think we've largely gone the wrong direction
> in that respect with respect to foreign tables and matviews.

Hmm, "hiding" would not be an accurate description of the proposal. I
would characterize it more as removing extraneous information, since
for a partitioned table seeing 1000 records all with roughly the same
name isn't helpful output from \d

\d would show tables but not partitions
\d  would show partitions exist and how many
\d+ would show partition details

So the information would be available, just at different levels of
detail, just as we have now for other things.

-- 
Simon Riggshttp://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] New partitioning - some feedback

[Tom Lane]

Robert Haas  writes:
> I don't have a strong view on whether partitions should be hidden by
> default, although I lean slightly against it (say, -0.25).  But if we
> do decide to hide them by default, then I definitely want an
> easy-to-use modifier that overrides that behavior, like being able to
> type \d! or whatever to have them included after all.

AIUI the user is responsible for DDL on partitions, like say creating
indexes for them?  Seems like hiding them is a bad idea given that.
Also, we need to be careful about calling them something very separate
from "table", because that would rouse the need to have duplicate syntax
for every sort of ALTER TABLE and suchlike command that we want to have
be usable with partitions.  I think we've largely gone the wrong direction
in that respect with respect to foreign tables and matviews.

regards, tom lane


-- 
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] New partitioning - some feedback

[Robert Haas]

On Fri, Jul 7, 2017 at 3:54 AM, Michael Banck  wrote:
> +1.
>
> Or maybe just 'partition' is enough if 'partition table' would widen the
> column output unnecessarily.

Internally to the source code, the parent is called a "partitioned
table" and the child is called a "partition".  I think we should not
use the term "partition table" because I think it could create
confusion as to which of those two things we're talking about.  It
would be reasonable to write "partition" rather than "table" for
partitions, though.  We'd probably also need "partition index" (for
indexes on partition) and "foreign partition" (for foreign tables that
are partitions).

I don't have a strong view on whether partitions should be hidden by
default, although I lean slightly against it (say, -0.25).  But if we
do decide to hide them by default, then I definitely want an
easy-to-use modifier that overrides that behavior, like being able to
type \d! or whatever to have them included after all.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] New partitioning - some feedback

[Simon Riggs]

On 7 July 2017 at 08:54, Michael Banck  wrote:
> On Fri, Jul 07, 2017 at 07:40:55PM +1200, Mark Kirkwood wrote:
>> On 07/07/17 13:29, Amit Langote wrote:
>> >Someone complained about this awhile back [1].  And then it came up again
>> >[2], where Noah appeared to take a stance that partitions should be
>> >visible in views / output of commands that list "tables".
>> >
>> >Although I too tend to prefer not filling up the \d output space by
>> >listing partitions (pg_class.relispartition = true relations), there
>> >wasn't perhaps enough push for creating a patch for that.  If some
>> >committer is willing to consider such a patch, I can make one.
>>
>> Yeah, me too (clearly). However if the consensus is that all these partition
>> tables *must* be shown in \d output, then I'd be happy if they were
>> identified as such rather than just 'table' (e.g 'partition table').
>
> +1.

+1 to remove partitions from \d display

With 1000 partitions that would just be annoying

-- 
Simon Riggshttp://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] New partitioning - some feedback

[Michael Banck]

On Fri, Jul 07, 2017 at 07:40:55PM +1200, Mark Kirkwood wrote:
> On 07/07/17 13:29, Amit Langote wrote:
> >Someone complained about this awhile back [1].  And then it came up again
> >[2], where Noah appeared to take a stance that partitions should be
> >visible in views / output of commands that list "tables".
> >
> >Although I too tend to prefer not filling up the \d output space by
> >listing partitions (pg_class.relispartition = true relations), there
> >wasn't perhaps enough push for creating a patch for that.  If some
> >committer is willing to consider such a patch, I can make one.
> 
> Yeah, me too (clearly). However if the consensus is that all these partition
> tables *must* be shown in \d output, then I'd be happy if they were
> identified as such rather than just 'table' (e.g 'partition table').

+1.

Or maybe just 'partition' is enough if 'partition table' would widen the
column output unnecessarily.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] New partitioning - some feedback

[Mark Kirkwood]

On 07/07/17 13:29, Amit Langote wrote:



Someone complained about this awhile back [1].  And then it came up again
[2], where Noah appeared to take a stance that partitions should be
visible in views / output of commands that list "tables".

Although I too tend to prefer not filling up the \d output space by
listing partitions (pg_class.relispartition = true relations), there
wasn't perhaps enough push for creating a patch for that.  If some
committer is willing to consider such a patch, I can make one.




Yeah, me too (clearly). However if the consensus is that all these 
partition tables *must* be shown in \d output, then I'd be happy if they 
were identified as such rather than just 'table' (e.g 'partition table').


regards

Mark


--
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] New partitioning - some feedback

[Amit Langote]

Hi Mark,

On 2017/07/07 9:02, Mark Kirkwood wrote:
> I've been trying out the new partitioning in version 10. Firstly, I must
> say this is excellent - so much nicer than the old inheritance based method!

Thanks. :)

> My only niggle is the display of partitioned tables via \d etc. e.g:
>
> part=# \d
> List of relations
>  Schema | Name | Type  |  Owner
> +--+---+--
>  public | date_fact| table | postgres
>  public | date_fact_201705 | table | postgres
>  public | date_fact_201706 | table | postgres
>  public | date_fact_20170601   | table | postgres
>  public | date_fact_2017060100 | table | postgres
>  public | date_fact_201707 | table | postgres
>  public | date_fact_rest   | table | postgres
> (7 rows)
> 
> Now it can be inferred from the names that date_fact is a partitioned
> table and the various date_fact_ are its partitions - but \d is not
> providing any hints of this. The more detailed individual describe is fine:
> 
> part=# \d date_fact
>   Table "public.date_fact"
>  Column |   Type   | Collation | Nullable | Default
> +--+---+--+-
>  id | integer  |   | not null |
>  dte| timestamp with time zone |   | not null |
>  val| integer  |   | not null |
> Partition key: RANGE (dte)
> Number of partitions: 6 (Use \d+ to list them.)
> 
> I'd prefer *not* to see a table and its partitions all intermixed in the
> same display (especially with nothing indicating which are partitions) -
> as this will make for unwieldy long lists when tables have many
> partitions. Also it would be good if the 'main' partitioned table and its
> 'partitions' showed up as a different type in some way.
> I note the they do in pg_class:
> 
> part=# SELECT relname,relkind,relispartition FROM pg_class WHERE relname
> LIKE 'date_fact%';
>relname| relkind | relispartition
> --+-+
>  date_fact| p   | f
>  date_fact_201705 | r   | t
>  date_fact_201706 | r   | t
>  date_fact_20170601   | r   | t
>  date_fact_2017060100 | r   | t
>  date_fact_201707 | r   | t
>  date_fact_rest   | r   | t
> (7 rows)
> 
> ...so it looks to be possible to hide the partitions from the main display
> and/or mark them as such. Now I realize that making this comment now that
> beta is out is a bit annoying - apologies, but I think seeing a huge list
> of 'tables' is going to make \d frustrating for folk doing partitioning.

Someone complained about this awhile back [1].  And then it came up again
[2], where Noah appeared to take a stance that partitions should be
visible in views / output of commands that list "tables".

Although I too tend to prefer not filling up the \d output space by
listing partitions (pg_class.relispartition = true relations), there
wasn't perhaps enough push for creating a patch for that.  If some
committer is willing to consider such a patch, I can make one.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAM-w4HOZ5fPS7GoCTTrW42q01%2BwPrOWFCnr9H0iDyVTZP2H1CA%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/20170406070227.GA2741046%40tornado.leadboat.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] New partitioning - some feedback

[Mark Kirkwood]

I've been trying out the new partitioning in version 10. Firstly, I must 
say this is excellent - so much nicer than the old inheritance based method!


My only niggle is the display of partitioned tables via \d etc. e.g:

part=# \d
List of relations
 Schema | Name | Type  |  Owner
+--+---+--
 public | date_fact| table | postgres
 public | date_fact_201705 | table | postgres
 public | date_fact_201706 | table | postgres
 public | date_fact_20170601   | table | postgres
 public | date_fact_2017060100 | table | postgres
 public | date_fact_201707 | table | postgres
 public | date_fact_rest   | table | postgres
(7 rows)

Now it can be inferred from the names that date_fact is a partitioned 
table and the various date_fact_ are its partitions - but \d is not 
providing any hints of this. The more detailed individual describe is fine:


part=# \d date_fact
  Table "public.date_fact"
 Column |   Type   | Collation | Nullable | Default
+--+---+--+-
 id | integer  |   | not null |
 dte| timestamp with time zone |   | not null |
 val| integer  |   | not null |
Partition key: RANGE (dte)
Number of partitions: 6 (Use \d+ to list them.)

I'd prefer *not* to see a table and its partitions all intermixed in the 
same display (especially with nothing indicating which are partitions) - 
as this will make for unwieldy long lists when tables have many 
partitions. Also it would be good if the 'main' partitioned table and 
its 'partitions' showed up as a different type in some way.


I note the they do in pg_class:

part=# SELECT relname,relkind,relispartition FROM pg_class WHERE relname 
LIKE 'date_fact%';

   relname| relkind | relispartition
--+-+
 date_fact| p   | f
 date_fact_201705 | r   | t
 date_fact_201706 | r   | t
 date_fact_20170601   | r   | t
 date_fact_2017060100 | r   | t
 date_fact_201707 | r   | t
 date_fact_rest   | r   | t
(7 rows)

...so it looks to be possible to hide the partitions from the main 
display and/or mark them as such. Now I realize that making this comment 
now that beta is out is a bit annoying - apologies, but I think seeing a 
huge list of 'tables' is going to make \d frustrating for folk doing 
partitioning.


regards

Mark



--
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] Declarative partitioning - another take

[Etsuro Fujita]

Hi Maksim,

On 2017/04/07 19:52, Maksim Milyutin wrote:

On 07.04.2017 13:05, Etsuro Fujita wrote:



On 2016/12/09 19:46, Maksim Milyutin wrote:

I would like to work on two tasks:
 - insert (and eventually update) tuple routing for foreign partition.
 - the ability to create an index on the parent and have all of the
children inherit it;



There seems to be no work on the first one, so I'd like to work on that.



Yes, you can start to work on this, I'll join later as a reviewer.


Great!  I added the patch to the next commitfest:

https://commitfest.postgresql.org/14/1184/

Sorry for the delay.

Best regards,
Etsuro Fujita



--
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] Declarative partitioning - another take

[Robert Haas]

On Wed, May 10, 2017 at 6:50 AM, Etsuro Fujita
 wrote:
> I started working on this.  I agree that the changes made in
> make_modifytable would be unacceptable, but I'd vote for Amit's idea of
> passing a modified PlannerInfo to PlanForeignModify so that the FDW can do
> query planning for INSERT into a foreign partition in the same way as for
> INSERT into a non-partition foreign table.  (Though, I think we should
> generate a more-valid-looking working-copy of the PlannerInfo which has
> Query with the foreign partition as target.)  I'm not sure it's a good idea
> to add a new FDW API or modify the existing one such as PlanForeignModify
> for this purpose.

Thanks for working on this, but I think it would be better to start a
new thread for this discussion.

And probably also for any other issues that come up.  This thread has
gotten so long that between the original discussion and commit of the
patch and discussion of multiple follow-on commits and patches that
it's very hard to follow.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Etsuro Fujita]

On 2016/11/18 1:43, Robert Haas wrote:

On Thu, Nov 17, 2016 at 6:27 AM, Amit Langote
 wrote:



- The code in make_modifytable() to swap out the rte_array for a fake
one looks like an unacceptable kludge.  I don't know offhand what a
better design would look like, but what you've got is really ugly.



Agree that it looks horrible.  The problem is we don't add partition
(child table) RTEs when planning an insert on the parent and FDW
partitions can't do without some planner handling - planForeignModify()
expects a valid PlannerInfo for deparsing target lists (basically, to be
able to use planner_rt_fetch()).



If it's only needed for foreign tables, how about for v1 we just throw
an error and say errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot route inserted tuples to a foreign table") for now.  We
can come back and fix it later.  Doing more inheritance expansion



Coming up with some new FDW API or some modification
to the existing one is probably better, but I don't really want to get
hung up on that right now.


I started working on this.  I agree that the changes made in 
make_modifytable would be unacceptable, but I'd vote for Amit's idea of 
passing a modified PlannerInfo to PlanForeignModify so that the FDW can 
do query planning for INSERT into a foreign partition in the same way as 
for INSERT into a non-partition foreign table.  (Though, I think we 
should generate a more-valid-looking working-copy of the PlannerInfo 
which has Query with the foreign partition as target.)  I'm not sure 
it's a good idea to add a new FDW API or modify the existing one such as 
PlanForeignModify for this purpose.


Best regards,
Etsuro Fujita



--
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] Declarative partitioning - another take

[Amit Langote]

On 2017/05/10 12:59, Robert Haas wrote:
> On Tue, May 9, 2017 at 11:54 PM, Thomas Munro
>  wrote:
>> +A statement that targets a parent table in a inheritance or partitioning
>>
>> A tiny typo: s/a inheritance/an inheritance/
> 
> Now he tells me.

Thanks both.

Regards,
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] Declarative partitioning - another take

[Robert Haas]

On Tue, May 9, 2017 at 11:54 PM, Thomas Munro
 wrote:
> +A statement that targets a parent table in a inheritance or partitioning
>
> A tiny typo: s/a inheritance/an inheritance/

Now he tells me.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Thomas Munro]

On Wed, May 10, 2017 at 3:51 PM, Robert Haas  wrote:
> On Sun, May 7, 2017 at 9:44 PM, Amit Langote
>  wrote:
>> I think that makes sense.  Modified it to read: "A statement that targets
>> a parent table in a inheritance or partitioning hierarchy..." in the
>> attached updated patch.
>
> LGTM.  Committed.

+A statement that targets a parent table in a inheritance or partitioning

A tiny typo: s/a inheritance/an inheritance/

-- 
Thomas Munro
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] Declarative partitioning - another take

[Robert Haas]

On Sun, May 7, 2017 at 9:44 PM, Amit Langote
 wrote:
> I think that makes sense.  Modified it to read: "A statement that targets
> a parent table in a inheritance or partitioning hierarchy..." in the
> attached updated patch.

LGTM.  Committed.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2017/05/08 10:22, Thomas Munro wrote:
> On Mon, May 8, 2017 at 12:47 PM, Amit Langote
>  wrote:
>> On 2017/05/03 2:48, Robert Haas wrote:
>>> On Tue, May 2, 2017 at 3:30 AM, Amit Langote
>>>  wrote:
 You're right.  I agree that whatever text we add here should be pointing
 out that statement-level triggers of affected child tables are not fired,
 when root parent is specified in the command.

 Since there was least some talk of changing that behavior for regular
 inheritance so that statement triggers of any affected children are fired
 [1], I thought we shouldn't say something general that applies to both
 inheritance and partitioning.  But since nothing has happened in that
 regard, we might as well.

 How about the attached?
>>>
>>> Looks better, but I think we should say "statement" instead of
>>> "operation" for consistency with the previous paragraph, and it
>>> certainly shouldn't be capitalized.
>>
>> Agreed, done.  Attached updated patch.
> 
> 
> +A statement that targets the root table in a inheritance or partitioning
> +hierarchy does not cause the statement-level triggers of affected child
> +tables to be fired; only the root table's statement-level triggers are
> +fired.  However, row-level triggers of any affected child tables will be
> +fired.
> +   
> +
> +   
> 
> Why talk specifically about the "root" table?  Wouldn't we describe
> the situation more generally if we said [a,the] "parent"?

I think that makes sense.  Modified it to read: "A statement that targets
a parent table in a inheritance or partitioning hierarchy..." in the
attached updated patch.

Thanks,
Amit
>From 5c2c453235d5eedf857a5e7123337aae5aedd761 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH] Clarify statement trigger behavior with inheritance

---
 doc/src/sgml/trigger.sgml | 8 
 1 file changed, 8 insertions(+)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 6f8416dda7..ce76a1f042 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -123,6 +123,14 @@

 

+A statement that targets a parent table in a inheritance or partitioning
+hierarchy does not cause the statement-level triggers of affected child
+tables to be fired; only the parent table's statement-level triggers are
+fired.  However, row-level triggers of any affected child tables will be
+fired.
+   
+
+   
 If an INSERT contains an ON CONFLICT
 DO UPDATE clause, it is possible that the effects of all
 row-level BEFORE INSERT triggers
-- 
2.11.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] Declarative partitioning - another take

[Thomas Munro]

On Mon, May 8, 2017 at 12:47 PM, Amit Langote
 wrote:
> On 2017/05/03 2:48, Robert Haas wrote:
>> On Tue, May 2, 2017 at 3:30 AM, Amit Langote
>>  wrote:
>>> You're right.  I agree that whatever text we add here should be pointing
>>> out that statement-level triggers of affected child tables are not fired,
>>> when root parent is specified in the command.
>>>
>>> Since there was least some talk of changing that behavior for regular
>>> inheritance so that statement triggers of any affected children are fired
>>> [1], I thought we shouldn't say something general that applies to both
>>> inheritance and partitioning.  But since nothing has happened in that
>>> regard, we might as well.
>>>
>>> How about the attached?
>>
>> Looks better, but I think we should say "statement" instead of
>> "operation" for consistency with the previous paragraph, and it
>> certainly shouldn't be capitalized.
>
> Agreed, done.  Attached updated patch.


+A statement that targets the root table in a inheritance or partitioning
+hierarchy does not cause the statement-level triggers of affected child
+tables to be fired; only the root table's statement-level triggers are
+fired.  However, row-level triggers of any affected child tables will be
+fired.
+   
+
+   

Why talk specifically about the "root" table?  Wouldn't we describe
the situation more generally if we said [a,the] "parent"?

-- 
Thomas Munro
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] Declarative partitioning - another take

[Amit Langote]

On 2017/05/03 2:48, Robert Haas wrote:
> On Tue, May 2, 2017 at 3:30 AM, Amit Langote
>  wrote:
>> You're right.  I agree that whatever text we add here should be pointing
>> out that statement-level triggers of affected child tables are not fired,
>> when root parent is specified in the command.
>>
>> Since there was least some talk of changing that behavior for regular
>> inheritance so that statement triggers of any affected children are fired
>> [1], I thought we shouldn't say something general that applies to both
>> inheritance and partitioning.  But since nothing has happened in that
>> regard, we might as well.
>>
>> How about the attached?
> 
> Looks better, but I think we should say "statement" instead of
> "operation" for consistency with the previous paragraph, and it
> certainly shouldn't be capitalized.

Agreed, done.  Attached updated patch.

Thanks,
Amit
>From 1d7e383c6d89ebabacc7aa3f7d9987779daaa4fb Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH] Clarify statement trigger behavior with inheritance

---
 doc/src/sgml/trigger.sgml | 8 
 1 file changed, 8 insertions(+)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 6f8416dda7..89e8c01a71 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -123,6 +123,14 @@

 

+A statement that targets the root table in a inheritance or partitioning
+hierarchy does not cause the statement-level triggers of affected child
+tables to be fired; only the root table's statement-level triggers are
+fired.  However, row-level triggers of any affected child tables will be
+fired.
+   
+
+   
 If an INSERT contains an ON CONFLICT
 DO UPDATE clause, it is possible that the effects of all
 row-level BEFORE INSERT triggers
-- 
2.11.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] Declarative partitioning - another take

[Robert Haas]

On Tue, May 2, 2017 at 3:30 AM, Amit Langote
 wrote:
> You're right.  I agree that whatever text we add here should be pointing
> out that statement-level triggers of affected child tables are not fired,
> when root parent is specified in the command.
>
> Since there was least some talk of changing that behavior for regular
> inheritance so that statement triggers of any affected children are fired
> [1], I thought we shouldn't say something general that applies to both
> inheritance and partitioning.  But since nothing has happened in that
> regard, we might as well.
>
> How about the attached?

Looks better, but I think we should say "statement" instead of
"operation" for consistency with the previous paragraph, and it
certainly shouldn't be capitalized.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2017/05/01 21:30, Robert Haas wrote:
> On Mon, May 1, 2017 at 12:18 AM, Amit Langote
>  wrote:
>> Attached updated patch.
> 
> Committed, except for this bit:

Thanks.

> +A statement-level trigger defined on partitioned tables is fired only
> +once for the table itself, not once for every table in the partitioning
> +hierarchy.  However, row-level triggers of any affected leaf partitions
> +will be fired.
> 
> The first sentence there has a number of issues.  Grammatically, there
> is an agreement problem: trigger is singular, but partitioned table is
> plural, and one trigger isn't defined across multiple tables.  It
> would have to say something like "A statement-level trigger defined on
> a partitioned table".  But even with that correction, it's not really
> saying what you want it to say.  Nobody would expect that the same
> statement-level trigger would be fired multiple times.  The issue is
> whether statement-level triggers on the partitions themselves will be
> fired, not the statement-level trigger on the partitioned table.
> Also, if this applies to inheritance as well as partitioning, then why
> mention only partitioning?  I think we might want to document
> something more here, but not like this.

You're right.  I agree that whatever text we add here should be pointing
out that statement-level triggers of affected child tables are not fired,
when root parent is specified in the command.

Since there was least some talk of changing that behavior for regular
inheritance so that statement triggers of any affected children are fired
[1], I thought we shouldn't say something general that applies to both
inheritance and partitioning.  But since nothing has happened in that
regard, we might as well.

How about the attached?

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/cd282adde5b70b20c57f53bb9ab75...@biglumber.com
>From d2d27ca458fbd341efd5ae231f977385a5e3c951 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH] Clarify statement trigger behavior with inheritance

---
 doc/src/sgml/trigger.sgml | 8 
 1 file changed, 8 insertions(+)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 6f8416dda7..ef41085744 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -123,6 +123,14 @@

 

+An Operation that targets the root table in a inheritance or partitioning
+hierarchy does not cause the statement-level triggers of affected child
+tables to be fired; only the root table's statement-level triggers are
+fired.  However, row-level triggers of any affected child tables will be
+fired.
+   
+
+   
 If an INSERT contains an ON CONFLICT
 DO UPDATE clause, it is possible that the effects of all
 row-level BEFORE INSERT triggers
-- 
2.11.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] Declarative partitioning - another take

[Robert Haas]

On Mon, May 1, 2017 at 12:18 AM, Amit Langote
 wrote:
> Attached updated patch.

Committed, except for this bit:

+A statement-level trigger defined on partitioned tables is fired only
+once for the table itself, not once for every table in the partitioning
+hierarchy.  However, row-level triggers of any affected leaf partitions
+will be fired.

The first sentence there has a number of issues.  Grammatically, there
is an agreement problem: trigger is singular, but partitioned table is
plural, and one trigger isn't defined across multiple tables.  It
would have to say something like "A statement-level trigger defined on
a partitioned table".  But even with that correction, it's not really
saying what you want it to say.  Nobody would expect that the same
statement-level trigger would be fired multiple times.  The issue is
whether statement-level triggers on the partitions themselves will be
fired, not the statement-level trigger on the partitioned table.
Also, if this applies to inheritance as well as partitioning, then why
mention only partitioning?  I think we might want to document
something more here, but not like this.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2017/04/29 6:56, David Fetter wrote:
> On Fri, Apr 28, 2017 at 06:29:48PM +0900, Amit Langote wrote:
>> On 2017/04/28 7:36, David Fetter wrote:
>>> Did I notice correctly that there's no way to handle transition tables
>>> for partitions in AFTER STATEMENT triggers?
>>
>> Did you mean to ask about AFTER STATEMENT triggers defined on
>> "partitioned" tables?  Specifying transition table for them is disallowed
>> at all.
>>
>> ERROR:  "p" is a partitioned table
>> DETAIL:  Triggers on partitioned tables cannot have transition tables.
> 
> OK, I suppose.  It wasn't clear from the documentation.
> 
>> Triggers created on (leaf) partitions *do* allow specifying transition table.
> 
> That includes the upcoming "default" tables, I presume.

If a "default" table is also a "leaf" table, then yes.  A default
table/partition can also be itself a partitioned table, in which case, it
won't allow triggers that require transition tables.  AFAICS, it's the
table's being partitioned that stops it from supporting transition tables,
not whether it is a "default" partition or not.

>> Or are you asking something else altogether?
> 
> I was just fuzzy on the interactions among these features.
> 
>>> If not, I'm not suggesting that this be added at this late date, but
>>> we might want to document that.
>>
>> I don't see mentioned in the documentation that such triggers cannot be
>> defined on partitioned tables.  Is that what you are saying should be
>> documented?
> 
> Yes, but I bias toward documenting a lot, and this restriction could
> go away in some future version, which would make things more confusing
> in the long run.

Yeah, it would be a good idea to document this.

> I'm picturing a conversation in 2020 that goes
> something like this:
> 
> "On 10, you could have AFTER STATEMENT triggers on tables, foreigh
> tables, and leaf partition tables which referenced transition tables,
> but not on DEFAULT partitions.  On 11, you could on DEFAULT partition
> tables.  From 12 onward, you can have transition tables on any
> relation."

What we could document now is that partitioned tables don't allow
specifying triggers that reference transition tables.  Although, I am
wondering where this note really belongs - the partitioning chapter, the
triggers chapter or the CREATE TRIGGER reference page?  Maybe, Kevin and
Thomas have something to say about that.  If it turns out that the
partitioning chapter is a good place, here is a documentation patch.

Thanks,
Amit
>From 289b4d906abb50451392d0efe13926f710952ca0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 1 May 2017 13:46:58 +0900
Subject: [PATCH] Document transition table trigger limitation of partitioned
 tables

---
 doc/src/sgml/ddl.sgml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 84c4f20990..5f5a956d41 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3293,7 +3293,9 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
  
   
Row triggers, if necessary, must be defined on individual partitions,
-   not the partitioned table.
+   not the partitioned tables.  Also, triggers that require accessing
+   transition tables are not allowed to be defined on the partitioned
+   tables.
   
  
 
-- 
2.11.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] Declarative partitioning - another take

[Amit Langote]

Thanks for the review.

On 2017/04/29 1:25, Robert Haas wrote:
>  On Fri, Apr 28, 2017 at 2:13 AM, Amit Langote
>  wrote:
 By the way, code changes I made in the attached are such that a subsequent
 patch could implement firing statement-level triggers of all the tables in
 a partition hierarchy, which it seems we don't want to do.  Should then
 the code be changed to not create ResultRelInfos of all the tables but
 only the root table (the one mentioned in the command)?  You will see that
 the patch adds fields named es_nonleaf_result_relations and
 es_num_nonleaf_result_relations, whereas just es_root_result_relation
 would perhaps do, for example.
>>>
>>> It seems better not to create any ResultRelInfos that we don't
>>> actually need, so +1 for such a revision to the patch.
>>
>> OK, done.  It took a bit more work than I thought.
> 
> So, this seems weird, because rootResultRelIndex is initialized even
> when splan->partitioned_rels == NIL, but isn't actually valid in that
> case.  ExecInitModifyTable seems to think it's always valid, though.

OK, rootResultRelIndex is now set to a value >= 0 only when the node
modifies a partitioned table.  And then ExecInitModifyTable() checks if
the index is valid before initializing the root table info.

> I think the way that you've refactored fireBSTriggers and
> fireASTriggers is a bit confusing.  Instead of splitting out a
> separate function, how about just having the existing function begin
> with if (node->rootResultRelInfo) resultRelInfo =
> node->rootResultRelInfo; else resultRelInfo = node->resultRelInfo; ?
> I think the way you've coded it is a holdover from the earlier design
> where you were going to call it multiple times, but now that's not
> needed.

OK, done that way.

> Looks OK, otherwise.

Attached updated patch.

Thanks,
Amit
>From 53c964f9f1fd3e27824080b0e9e5b672fa53cdf0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH] Fire per-statement triggers of partitioned tables

Current implementation completely misses them, because it assumes
that no ResultRelInfos need to be created for partitioned tables,
whereas they *are* needed to fire the per-statment triggers, which
we *do* allow to be defined on the partitioned tables.

Reported By: Rajkumar Raghuwanshi
Patch by: Amit Langote
Report: https://www.postgresql.org/message-id/CAKcux6%3DwYospCRY2J4XEFuVy0L41S%3Dfic7rmkbsU-GXhhSbmBg%40mail.gmail.com
---
 doc/src/sgml/trigger.sgml   | 26 +++
 src/backend/executor/execMain.c | 55 --
 src/backend/executor/nodeModifyTable.c  | 42 +
 src/backend/nodes/copyfuncs.c   |  2 +
 src/backend/nodes/outfuncs.c|  3 ++
 src/backend/nodes/readfuncs.c   |  2 +
 src/backend/optimizer/plan/createplan.c |  1 +
 src/backend/optimizer/plan/planner.c|  3 ++
 src/backend/optimizer/plan/setrefs.c| 19 ++--
 src/include/nodes/execnodes.h   | 12 +
 src/include/nodes/plannodes.h   | 13 +-
 src/include/nodes/relation.h|  1 +
 src/test/regress/expected/triggers.out  | 81 +
 src/test/regress/sql/triggers.sql   | 70 
 14 files changed, 303 insertions(+), 27 deletions(-)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 2a718d7f47..5771bd5fdc 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -33,7 +33,8 @@

 A trigger is a specification that the database should automatically
 execute a particular function whenever a certain type of operation is
-performed.  Triggers can be attached to tables, views, and foreign tables.
+performed.  Triggers can be attached to tables (partitioned or not),
+views, and foreign tables.
   
 
   
@@ -111,14 +112,21 @@
 Statement-level BEFORE triggers naturally fire before the
 statement starts to do anything, while statement-level AFTER
 triggers fire at the very end of the statement.  These types of
-triggers may be defined on tables or views.  Row-level BEFORE
-triggers fire immediately before a particular row is operated on,
-while row-level AFTER triggers fire at the end of the
-statement (but before any statement-level AFTER triggers).
-These types of triggers may only be defined on tables and foreign tables.
-Row-level INSTEAD OF triggers may only be defined on views,
-and fire immediately as each row in the view is identified as needing to
-be operated on.
+triggers may be defined on tables, views, or foreign tables.  Row-level
+BEFORE triggers fire immediately before a particular row is
+operated on, while row-level AFTER triggers fire at the end of
+the statement (but before any statement-level AFTER triggers).
+These types of triggers may only be defined on non-partitioned tables and

Re: [HACKERS] Declarative partitioning - another take

[Noah Misch]

On Mon, Apr 24, 2017 at 07:43:29PM +0900, Amit Langote wrote:
> On 2017/04/21 17:00, Rajkumar Raghuwanshi wrote:
> > I am able to create statement triggers at root partition, but these
> > triggers, not getting fired on updating partition.

> It would be great if you could check if the patches fix the issues.
> 
> Also, adding this to the PostgreSQL 10 open items list.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] Declarative partitioning - another take

[David Fetter]

On Fri, Apr 28, 2017 at 06:29:48PM +0900, Amit Langote wrote:
> On 2017/04/28 7:36, David Fetter wrote:
> > On Thu, Apr 27, 2017 at 10:30:54AM +0900, Amit Langote wrote:
> >> On 2017/04/27 1:52, Robert Haas wrote:
> >>> On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
> >>>  wrote:
>  FWIW, I too prefer the latter, that is, fire only the parent's triggers.
>  In that case, applying only the patch 0001 will do.
> >>>
> >>> Do we need to update the documentation?
> >>
> >> Yes, I think we should.  How about as in the attached?
> >>
> >> By the way, code changes I made in the attached are such that a subsequent
> >> patch could implement firing statement-level triggers of all the tables in
> >> a partition hierarchy, which it seems we don't want to do.  Should then
> >> the code be changed to not create ResultRelInfos of all the tables but
> >> only the root table (the one mentioned in the command)?  You will see that
> >> the patch adds fields named es_nonleaf_result_relations and
> >> es_num_nonleaf_result_relations, whereas just es_root_result_relation
> >> would perhaps do, for example.
> > 
> > Did I notice correctly that there's no way to handle transition tables
> > for partitions in AFTER STATEMENT triggers?
> 
> Did you mean to ask about AFTER STATEMENT triggers defined on
> "partitioned" tables?  Specifying transition table for them is disallowed
> at all.
> 
> ERROR:  "p" is a partitioned table
> DETAIL:  Triggers on partitioned tables cannot have transition tables.

OK, I suppose.  It wasn't clear from the documentation.

> Triggers created on (leaf) partitions *do* allow specifying transition table.

That includes the upcoming "default" tables, I presume.

> Or are you asking something else altogether?

I was just fuzzy on the interactions among these features.

> > If not, I'm not suggesting that this be added at this late date, but
> > we might want to document that.
> 
> I don't see mentioned in the documentation that such triggers cannot be
> defined on partitioned tables.  Is that what you are saying should be
> documented?

Yes, but I bias toward documenting a lot, and this restriction could
go away in some future version, which would make things more confusing
in the long run.  I'm picturing a conversation in 2020 that goes
something like this:

"On 10, you could have AFTER STATEMENT triggers on tables, foreigh
tables, and leaf partition tables which referenced transition tables,
but not on DEFAULT partitions.  On 11, you could on DEFAULT partition
tables.  From 12 onward, you can have transition tables on any
relation."

Kevin?  Thomas?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Declarative partitioning - another take

[Robert Haas]

 On Fri, Apr 28, 2017 at 2:13 AM, Amit Langote
 wrote:
> It seems to me that there is no difference in behavior between
> inheritance-based and declarative partitioning as far as statement-level
> triggers are concerned (at least currently).  In both the cases, we fire
> statement-level triggers only for the table specified in the command.

OK.

>>> By the way, code changes I made in the attached are such that a subsequent
>>> patch could implement firing statement-level triggers of all the tables in
>>> a partition hierarchy, which it seems we don't want to do.  Should then
>>> the code be changed to not create ResultRelInfos of all the tables but
>>> only the root table (the one mentioned in the command)?  You will see that
>>> the patch adds fields named es_nonleaf_result_relations and
>>> es_num_nonleaf_result_relations, whereas just es_root_result_relation
>>> would perhaps do, for example.
>>
>> It seems better not to create any ResultRelInfos that we don't
>> actually need, so +1 for such a revision to the patch.
>
> OK, done.  It took a bit more work than I thought.

So, this seems weird, because rootResultRelIndex is initialized even
when splan->partitioned_rels == NIL, but isn't actually valid in that
case.  ExecInitModifyTable seems to think it's always valid, though.

I think the way that you've refactored fireBSTriggers and
fireASTriggers is a bit confusing.  Instead of splitting out a
separate function, how about just having the existing function begin
with if (node->rootResultRelInfo) resultRelInfo =
node->rootResultRelInfo; else resultRelInfo = node->resultRelInfo; ?
I think the way you've coded it is a holdover from the earlier design
where you were going to call it multiple times, but now that's not
needed.

Looks OK, otherwise.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2017/04/28 7:36, David Fetter wrote:
> On Thu, Apr 27, 2017 at 10:30:54AM +0900, Amit Langote wrote:
>> On 2017/04/27 1:52, Robert Haas wrote:
>>> On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
>>>  wrote:
 FWIW, I too prefer the latter, that is, fire only the parent's triggers.
 In that case, applying only the patch 0001 will do.
>>>
>>> Do we need to update the documentation?
>>
>> Yes, I think we should.  How about as in the attached?
>>
>> By the way, code changes I made in the attached are such that a subsequent
>> patch could implement firing statement-level triggers of all the tables in
>> a partition hierarchy, which it seems we don't want to do.  Should then
>> the code be changed to not create ResultRelInfos of all the tables but
>> only the root table (the one mentioned in the command)?  You will see that
>> the patch adds fields named es_nonleaf_result_relations and
>> es_num_nonleaf_result_relations, whereas just es_root_result_relation
>> would perhaps do, for example.
> 
> Did I notice correctly that there's no way to handle transition tables
> for partitions in AFTER STATEMENT triggers?

Did you mean to ask about AFTER STATEMENT triggers defined on
"partitioned" tables?  Specifying transition table for them is disallowed
at all.

ERROR:  "p" is a partitioned table
DETAIL:  Triggers on partitioned tables cannot have transition tables.

Triggers created on (leaf) partitions *do* allow specifying transition table.

Or are you asking something else altogether?

> If not, I'm not suggesting that this be added at this late date, but
> we might want to document that.

I don't see mentioned in the documentation that such triggers cannot be
defined on partitioned tables.  Is that what you are saying should be
documented?

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] Declarative partitioning - another take

[Amit Langote]

Hi Rajkumar,

On 2017/04/28 17:11, Rajkumar Raghuwanshi wrote:
> On Fri, Apr 28, 2017 at 11:43 AM, Amit Langote <
>> Updated patch attached.
> 
> I have applied given patch, could see below behaviour with statement
> trigger.
> 
> When trying to delete value within partition range, triggers got fired
> (with zero row affected) as expected, but if trying to delete value which
> is outside of partition range (with zero row affected), No trigger fired.
> is this expected??

Good catch.

I'm afraid though that this is not a defect of this patch, but some
unrelated (maybe) bug, which affects not only the partitioned tables but
inheritance in general.

Problem is that the plan created is such that the executor has no
opportunity to fire the trigger in question, because the plan contains no
information about which table is affected by the statement.  You can see
that with inheritance.  See below:

create table foo ();
create table bar () inherits (foo);

create or replace function trig_notice() returns trigger as $$
  begin raise notice 'trigger fired'; return null; end;
$$ language plpgsql;

create trigger foo_del_before before delete on foo
  for each statement execute procedure trig_notice();

explain delete from foo where false;
QUERY PLAN
--
 Result  (cost=0.00..0.00 rows=0 width=0)
   One-Time Filter: false
(2 rows)

-- no trigger fired
delete from foo where false;
DELETE 0

Trigger *is* fired though, if inheritance is not used.

explain delete from only foo where false;
   QUERY PLAN
-
 Delete on foo  (cost=0.00..0.00 rows=0 width=0)
   ->  Result  (cost=0.00..0.00 rows=0 width=6)
 One-Time Filter: false
(3 rows)

delete from only foo where false;
NOTICE:  trigger fired
DELETE 0

I'm not sure how to go about fixing this, if at all.  Or to fix it at
least for partitioned tables.  Would like to hear what others 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] Declarative partitioning - another take

[Rajkumar Raghuwanshi]

On Fri, Apr 28, 2017 at 11:43 AM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:

> Updated patch attached.
>

Hi Amit,

I have applied given patch, could see below behaviour with statement
trigger.

When trying to delete value within partition range, triggers got fired
(with zero row affected) as expected, but if trying to delete value which
is outside of partition range (with zero row affected), No trigger fired.
is this expected??

Below are steps to reproduce.

CREATE TABLE trigger_test_table (a INT, b INT) PARTITION BY RANGE(a);
CREATE TABLE trigger_test_table1 PARTITION OF trigger_test_table FOR VALUES
FROM (0) to (6);
INSERT INTO trigger_test_table (a,b) SELECT i,i FROM generate_series(1,3)i;

CREATE TABLE trigger_test_statatics(TG_NAME varchar,TG_TABLE_NAME
varchar,TG_LEVEL varchar,TG_WHEN varchar, TG_OP varchar);

CREATE FUNCTION trigger_test_procedure() RETURNS TRIGGER AS $ttp$
BEGIN
INSERT INTO trigger_test_statatics SELECT
TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN,TG_OP;
RETURN OLD;
END;
$ttp$ LANGUAGE plpgsql;

CREATE TRIGGER trigger_test11 AFTER DELETE ON trigger_test_table FOR EACH
STATEMENT EXECUTE PROCEDURE trigger_test_procedure();
CREATE TRIGGER trigger_test12 AFTER DELETE ON trigger_test_table1 FOR EACH
STATEMENT EXECUTE PROCEDURE trigger_test_procedure();

postgres=# DELETE FROM trigger_test_table WHERE a = 5;
DELETE 0
postgres=# SELECT * FROM trigger_test_statatics;
tg_name |   tg_table_name| tg_level  | tg_when | tg_op
++---+-+
 trigger_test11 | trigger_test_table | STATEMENT | AFTER   | DELETE
(1 row)

TRUNCATE TABLE trigger_test_statatics;

--statement trigger NOT fired, when trying to delete data outside partition
range.
postgres=# DELETE FROM trigger_test_table WHERE a = 10;
DELETE 0
postgres=# SELECT * FROM trigger_test_statatics;
 tg_name | tg_table_name | tg_level | tg_when | tg_op
-+---+--+-+---
(0 rows)

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

Thanks for taking a look.

On 2017/04/28 5:24, Robert Haas wrote:
> On Wed, Apr 26, 2017 at 9:30 PM, Amit Langote
>  wrote:
>>> Do we need to update the documentation?
>>
>> Yes, I think we should.  How about as in the attached?
> 
> Looks reasonable, but I was thinking you might also update the section
> which contrasts inheritance-based partitioning with declarative
> partitioning.

It seems to me that there is no difference in behavior between
inheritance-based and declarative partitioning as far as statement-level
triggers are concerned (at least currently).  In both the cases, we fire
statement-level triggers only for the table specified in the command.

Maybe, we will fix things someday so that statement-level triggers will be
fired for all the tables in a inheritance hierarchy when the root parent
is updated or deleted, but that's currently not true.  We may never
implement that behavior for declarative partitioned tables though, so
there will be a difference if and when we implement the former.

Am I missing something?

>> By the way, code changes I made in the attached are such that a subsequent
>> patch could implement firing statement-level triggers of all the tables in
>> a partition hierarchy, which it seems we don't want to do.  Should then
>> the code be changed to not create ResultRelInfos of all the tables but
>> only the root table (the one mentioned in the command)?  You will see that
>> the patch adds fields named es_nonleaf_result_relations and
>> es_num_nonleaf_result_relations, whereas just es_root_result_relation
>> would perhaps do, for example.
> 
> It seems better not to create any ResultRelInfos that we don't
> actually need, so +1 for such a revision to the patch.

OK, done.  It took a bit more work than I thought.

Updated patch attached.

Thanks,
Amit
>From a8b584f09bc02b6c16c33e816fc12c7e443dd65c Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH] Fire per-statement triggers of partitioned tables

Current implementation completely misses them, because it assumes
that no ResultRelInfos need to be created for partitioned tables,
whereas they *are* needed to fire the per-statment triggers, which
we *do* allow to be defined on the partitioned tables.

Reported By: Rajkumar Raghuwanshi
Patch by: Amit Langote
Report: https://www.postgresql.org/message-id/CAKcux6%3DwYospCRY2J4XEFuVy0L41S%3Dfic7rmkbsU-GXhhSbmBg%40mail.gmail.com
---
 doc/src/sgml/trigger.sgml   | 26 +++
 src/backend/executor/execMain.c | 55 --
 src/backend/executor/nodeModifyTable.c  | 64 ++
 src/backend/nodes/copyfuncs.c   |  2 +
 src/backend/nodes/outfuncs.c|  3 ++
 src/backend/nodes/readfuncs.c   |  2 +
 src/backend/optimizer/plan/createplan.c |  1 +
 src/backend/optimizer/plan/planner.c|  3 ++
 src/backend/optimizer/plan/setrefs.c| 19 ++--
 src/include/nodes/execnodes.h   | 12 +
 src/include/nodes/plannodes.h   | 13 +-
 src/include/nodes/relation.h|  1 +
 src/test/regress/expected/triggers.out  | 81 +
 src/test/regress/sql/triggers.sql   | 70 
 14 files changed, 323 insertions(+), 29 deletions(-)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 2a718d7f47..5771bd5fdc 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -33,7 +33,8 @@

 A trigger is a specification that the database should automatically
 execute a particular function whenever a certain type of operation is
-performed.  Triggers can be attached to tables, views, and foreign tables.
+performed.  Triggers can be attached to tables (partitioned or not),
+views, and foreign tables.
   
 
   
@@ -111,14 +112,21 @@
 Statement-level BEFORE triggers naturally fire before the
 statement starts to do anything, while statement-level AFTER
 triggers fire at the very end of the statement.  These types of
-triggers may be defined on tables or views.  Row-level BEFORE
-triggers fire immediately before a particular row is operated on,
-while row-level AFTER triggers fire at the end of the
-statement (but before any statement-level AFTER triggers).
-These types of triggers may only be defined on tables and foreign tables.
-Row-level INSTEAD OF triggers may only be defined on views,
-and fire immediately as each row in the view is identified as needing to
-be operated on.
+triggers may be defined on tables, views, or foreign tables.  Row-level
+BEFORE triggers fire immediately before a particular row is
+operated on, while row-level AFTER triggers fire at the end of
+the statement (but before any statement-level AFTER triggers).
+These types of triggers may only be defined on non-partitioned tables and
+foreign tables.  

Re: [HACKERS] Declarative partitioning - another take

[David Fetter]

On Thu, Apr 27, 2017 at 10:30:54AM +0900, Amit Langote wrote:
> On 2017/04/27 1:52, Robert Haas wrote:
> > On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
> >  wrote:
> >> FWIW, I too prefer the latter, that is, fire only the parent's triggers.
> >> In that case, applying only the patch 0001 will do.
> > 
> > Do we need to update the documentation?
> 
> Yes, I think we should.  How about as in the attached?
> 
> By the way, code changes I made in the attached are such that a subsequent
> patch could implement firing statement-level triggers of all the tables in
> a partition hierarchy, which it seems we don't want to do.  Should then
> the code be changed to not create ResultRelInfos of all the tables but
> only the root table (the one mentioned in the command)?  You will see that
> the patch adds fields named es_nonleaf_result_relations and
> es_num_nonleaf_result_relations, whereas just es_root_result_relation
> would perhaps do, for example.

Did I notice correctly that there's no way to handle transition tables
for partitions in AFTER STATEMENT triggers?

If not, I'm not suggesting that this be added at this late date, but
we might want to document that.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Declarative partitioning - another take

[Robert Haas]

On Wed, Apr 26, 2017 at 9:30 PM, Amit Langote
 wrote:
>> Do we need to update the documentation?
>
> Yes, I think we should.  How about as in the attached?

Looks reasonable, but I was thinking you might also update the section
which contrasts inheritance-based partitioning with declarative
partitioning.

> By the way, code changes I made in the attached are such that a subsequent
> patch could implement firing statement-level triggers of all the tables in
> a partition hierarchy, which it seems we don't want to do.  Should then
> the code be changed to not create ResultRelInfos of all the tables but
> only the root table (the one mentioned in the command)?  You will see that
> the patch adds fields named es_nonleaf_result_relations and
> es_num_nonleaf_result_relations, whereas just es_root_result_relation
> would perhaps do, for example.

It seems better not to create any ResultRelInfos that we don't
actually need, so +1 for such a revision to the patch.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2017/04/27 1:52, Robert Haas wrote:
> On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
>  wrote:
>> FWIW, I too prefer the latter, that is, fire only the parent's triggers.
>> In that case, applying only the patch 0001 will do.
> 
> Do we need to update the documentation?

Yes, I think we should.  How about as in the attached?

By the way, code changes I made in the attached are such that a subsequent
patch could implement firing statement-level triggers of all the tables in
a partition hierarchy, which it seems we don't want to do.  Should then
the code be changed to not create ResultRelInfos of all the tables but
only the root table (the one mentioned in the command)?  You will see that
the patch adds fields named es_nonleaf_result_relations and
es_num_nonleaf_result_relations, whereas just es_root_result_relation
would perhaps do, for example.

Thanks,
Amit
>From c8b785a77a2a193906967762066e03e09de80442 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH 1/2] Fire per-statement triggers of partitioned tables

Current implementation completely misses them, because it assumes
that no ResultRelInfos need to be created for partitioned tables,
whereas they *are* needed to fire the per-statment triggers, which
we *do* allow to be defined on the partitioned tables.

Reported By: Rajkumar Raghuwanshi
Patch by: Amit Langote
Report: https://www.postgresql.org/message-id/CAKcux6%3DwYospCRY2J4XEFuVy0L41S%3Dfic7rmkbsU-GXhhSbmBg%40mail.gmail.com
---
 doc/src/sgml/trigger.sgml   | 26 -
 src/backend/executor/execMain.c | 33 -
 src/backend/executor/nodeModifyTable.c  | 66 -
 src/backend/nodes/copyfuncs.c   |  1 +
 src/backend/nodes/outfuncs.c|  1 +
 src/backend/nodes/readfuncs.c   |  1 +
 src/backend/optimizer/plan/createplan.c |  1 +
 src/backend/optimizer/plan/setrefs.c|  4 ++
 src/include/nodes/execnodes.h   | 11 ++
 src/include/nodes/plannodes.h   |  2 +
 src/test/regress/expected/triggers.out  | 54 +++
 src/test/regress/sql/triggers.sql   | 48 
 12 files changed, 227 insertions(+), 21 deletions(-)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 2a718d7f47..5771bd5fdc 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -33,7 +33,8 @@

 A trigger is a specification that the database should automatically
 execute a particular function whenever a certain type of operation is
-performed.  Triggers can be attached to tables, views, and foreign tables.
+performed.  Triggers can be attached to tables (partitioned or not),
+views, and foreign tables.
   
 
   
@@ -111,14 +112,21 @@
 Statement-level BEFORE triggers naturally fire before the
 statement starts to do anything, while statement-level AFTER
 triggers fire at the very end of the statement.  These types of
-triggers may be defined on tables or views.  Row-level BEFORE
-triggers fire immediately before a particular row is operated on,
-while row-level AFTER triggers fire at the end of the
-statement (but before any statement-level AFTER triggers).
-These types of triggers may only be defined on tables and foreign tables.
-Row-level INSTEAD OF triggers may only be defined on views,
-and fire immediately as each row in the view is identified as needing to
-be operated on.
+triggers may be defined on tables, views, or foreign tables.  Row-level
+BEFORE triggers fire immediately before a particular row is
+operated on, while row-level AFTER triggers fire at the end of
+the statement (but before any statement-level AFTER triggers).
+These types of triggers may only be defined on non-partitioned tables and
+foreign tables.  Row-level INSTEAD OF triggers may only be
+defined on views, and fire immediately as each row in the view is
+identified as needing to be operated on.
+   
+
+   
+A statement-level trigger defined on partitioned tables is fired only
+once for the table itself, not once for every table in the partitioning
+hierarchy.  However, row-level triggers of any affected leaf partitions
+will be fired.

 

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 5c12fb457d..586b396b3e 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -861,18 +861,35 @@ InitPlan(QueryDesc *queryDesc, int eflags)
 
 		/*
 		 * In the partitioned result relation case, lock the non-leaf result
-		 * relations too.  We don't however need ResultRelInfos for them.
+		 * relations too.  We also need ResultRelInfos so that per-statement
+		 * triggers defined on these relations can be fired.
 		 */
 		if (plannedstmt->nonleafResultRelations)
 		{
+			numResultRelations =
+			

Re: [HACKERS] Declarative partitioning - another take

[Robert Haas]

On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
 wrote:
> FWIW, I too prefer the latter, that is, fire only the parent's triggers.
> In that case, applying only the patch 0001 will do.

Do we need to update the documentation?

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Amit Khandekar]

On 26 April 2017 at 00:28, Robert Haas  wrote:
> So what I'd prefer -- on
> the totally unprincipled basis that it would let us improve
> performance in the future -- if you operate on a partition directly,
> you fire the partition's triggers, but if you operate on the parent,
> only the parent's triggers fire.

I would also opt for this behaviour.

Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


-- 
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] Declarative partitioning - another take

[Amit Langote]

Hi,

Thanks for testing.

On 2017/04/25 19:03, Rajkumar Raghuwanshi wrote:
> Thanks for looking into it. I have applied fixes and checked for triggers.
> I could see difference in behaviour of statement triggers for INSERT and
> UPDATE, for insert only root partition triggers are getting fired but for
> update root as well as child partition table triggers both getting fired.
> is this expected??

Yes, because I didn't implement anything for the insert case yet.  I posed
a question whether to fire partitions' per-statement triggers when
inserting data through the root table.

Robert replied [1] that it would be desirable to not fire partitions'
per-statement triggers if the root table is mentioned in the query; only
fire their per-row triggers if any.  It already works that way for
inserts, and applying only 0001 will get you the same for update/delete.
Patch 0002 is to enable firing partition's per-statement triggers even if
the root table is mentioned in the query, but it implemented the same only
for the update/delete cases.  If we decide that that's the right thing to
do, then I will implement the same behavior for the insert case too.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoatBYy8Hyi3cYR1rFrCkD2NM4ZLZcck4QDGvH%3DHddfDwA%40mail.gmail.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] Declarative partitioning - another take

[Amit Langote]

On 2017/04/26 3:58, Robert Haas wrote:
> On Mon, Apr 24, 2017 at 6:43 AM, Amit Langote
>  wrote:
>> The reason it doesn't work is that we do not allocate ResultRelInfos for
>> partitioned tables (not even for the root partitioned table in the
>> update/delete cases), because the current implementation assumes they are
>> not required.  That's fine only so long as we consider that no rows are
>> inserted into them, no indexes need to be updated, and that no row-level
>> triggers are to be fired.  But it misses the fact that we do allow
>> statement-level triggers on partitioned tables.  So, we should fix things
>> such that ResultRelInfos are allocated so that any statement-level
>> triggers are fired. But there are following questions to consider:
>>
>> 1. Should we consider only the root partitioned table or all partitioned
>>tables in a given hierarchy, including the child partitioned tables?
>>Note that this is related to a current limitation of updating/deleting
>>inheritance hierarchies that we do not currently fire per-statement
>>triggers of the child tables.  See the TODO item in wiki:
>>https://wiki.postgresql.org/wiki/Todo#Triggers, which says: "When
>>statement-level triggers are defined on a parent table, have them fire
>>only on the parent table, and fire child table triggers only where
>>appropriate"
>>
>> 2. Do we apply the same to inserts on the partitioned tables?  Since
>>insert on a partitioned table implicitly affects its partitions, one
>>might say that we would need to fire per-statement insert triggers of
>>all the partitions.
> 
> It seems to me that it doesn't make a lot of sense to fire the
> triggers for some tables involved in the hierarchy and not others.  I
> suppose the consistent thing to do here is to make sure that we fire
> the statement triggers for all tables in the partitioning hierarchy
> for all operations (insert, update, delete, etc.).
> 
> TBH, I don't like that very much.  I'd rather fire the triggers only
> for the table actually named in the query and skip all the others,
> mostly because it seems like it would be faster and less likely to
> block future optimizations; eventually, I'd like to consider not even
> locking the children we're not touching, but that's not going to work
> if we have to check them all for triggers.  So what I'd prefer -- on
> the totally unprincipled basis that it would let us improve
> performance in the future -- if you operate on a partition directly,
> you fire the partition's triggers, but if you operate on the parent,
> only the parent's triggers fire.

FWIW, I too prefer the latter, that is, fire only the parent's triggers.
In that case, applying only the patch 0001 will do.

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] Declarative partitioning - another take

[Robert Haas]

On Mon, Apr 24, 2017 at 6:43 AM, Amit Langote
 wrote:
> The reason it doesn't work is that we do not allocate ResultRelInfos for
> partitioned tables (not even for the root partitioned table in the
> update/delete cases), because the current implementation assumes they are
> not required.  That's fine only so long as we consider that no rows are
> inserted into them, no indexes need to be updated, and that no row-level
> triggers are to be fired.  But it misses the fact that we do allow
> statement-level triggers on partitioned tables.  So, we should fix things
> such that ResultRelInfos are allocated so that any statement-level
> triggers are fired. But there are following questions to consider:
>
> 1. Should we consider only the root partitioned table or all partitioned
>tables in a given hierarchy, including the child partitioned tables?
>Note that this is related to a current limitation of updating/deleting
>inheritance hierarchies that we do not currently fire per-statement
>triggers of the child tables.  See the TODO item in wiki:
>https://wiki.postgresql.org/wiki/Todo#Triggers, which says: "When
>statement-level triggers are defined on a parent table, have them fire
>only on the parent table, and fire child table triggers only where
>appropriate"
>
> 2. Do we apply the same to inserts on the partitioned tables?  Since
>insert on a partitioned table implicitly affects its partitions, one
>might say that we would need to fire per-statement insert triggers of
>all the partitions.

It seems to me that it doesn't make a lot of sense to fire the
triggers for some tables involved in the hierarchy and not others.  I
suppose the consistent thing to do here is to make sure that we fire
the statement triggers for all tables in the partitioning hierarchy
for all operations (insert, update, delete, etc.).

TBH, I don't like that very much.  I'd rather fire the triggers only
for the table actually named in the query and skip all the others,
mostly because it seems like it would be faster and less likely to
block future optimizations; eventually, I'd like to consider not even
locking the children we're not touching, but that's not going to work
if we have to check them all for triggers.  So what I'd prefer -- on
the totally unprincipled basis that it would let us improve
performance in the future -- if you operate on a partition directly,
you fire the partition's triggers, but if you operate on the parent,
only the parent's triggers fire.

How heretical is that idea?

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Rajkumar Raghuwanshi]

On Mon, Apr 24, 2017 at 4:13 PM, Amit Langote  wrote:

> Hi Rajkumar,
>
> It would be great if you could check if the patches fix the issues.
>

Hi Amit,

Thanks for looking into it. I have applied fixes and checked for triggers.
I could see difference in behaviour of statement triggers for INSERT and
UPDATE, for insert only root partition triggers are getting fired but for
update root as well as child partition table triggers both getting fired.
is this expected??

Below are steps to reproduce.

CREATE TABLE pt (a INT, b INT) PARTITION BY RANGE(a);
CREATE TABLE pt1 PARTITION OF pt FOR VALUES FROM (1) to (6);
CREATE TABLE pt2 PARTITION OF pt FOR VALUES FROM (6) to (11);
INSERT INTO pt (a,b) SELECT i,i FROM generate_series(1,7)i;

CREATE TABLE pt_trigger(TG_NAME varchar,TG_TABLE_NAME varchar,TG_LEVEL
varchar,TG_WHEN varchar,a_old int,a_new int,b_old int,b_new int);
CREATE FUNCTION process_pt_trigger() RETURNS TRIGGER AS $ttp$
 BEGIN
 IF (TG_OP = 'INSERT') THEN
  IF (TG_LEVEL = 'STATEMENT') THEN INSERT INTO pt_trigger
SELECT TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN,NULL,NULL,NULL,NULL; END IF;
  IF (TG_LEVEL = 'ROW') THEN INSERT INTO pt_trigger SELECT
TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN,NULL,NEW.a,NULL,NEW.b; END IF;
  RETURN NEW;
  END IF;
 IF (TG_OP = 'UPDATE') THEN
IF (TG_LEVEL = 'STATEMENT') THEN INSERT INTO pt_trigger
SELECT TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN,NULL,NULL,NULL,NULL; END IF;
IF (TG_LEVEL = 'ROW') THEN INSERT INTO pt_trigger SELECT
TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN,OLD.a,NEW.a,OLD.b,NEW.b; END IF;
 RETURN NEW;
 END IF;
 END;
$ttp$ LANGUAGE plpgsql;

CREATE TRIGGER trigger_test11 AFTER INSERT OR UPDATE ON pt FOR EACH
STATEMENT EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER trigger_test12 AFTER INSERT OR UPDATE ON pt1 FOR EACH
STATEMENT EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER trigger_test13 AFTER INSERT OR UPDATE ON pt2 FOR EACH
STATEMENT EXECUTE PROCEDURE process_pt_trigger();

CREATE TRIGGER trigger_test21 BEFORE INSERT OR UPDATE ON pt FOR EACH
STATEMENT EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER trigger_test22 BEFORE INSERT OR UPDATE ON pt1 FOR EACH
STATEMENT EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER trigger_test23 BEFORE INSERT OR UPDATE ON pt2 FOR EACH
STATEMENT EXECUTE PROCEDURE process_pt_trigger();

CREATE TRIGGER trigger_test32 AFTER INSERT OR UPDATE ON pt1 FOR EACH ROW
EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER trigger_test33 AFTER INSERT OR UPDATE ON pt2 FOR EACH ROW
EXECUTE PROCEDURE process_pt_trigger();

CREATE TRIGGER trigger_test42 BEFORE INSERT OR UPDATE ON pt1 FOR EACH ROW
EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER trigger_test43 BEFORE INSERT OR UPDATE ON pt2 FOR EACH ROW
EXECUTE PROCEDURE process_pt_trigger();

postgres=# INSERT INTO pt (a,b) VALUES (8,8);
INSERT 0 1
postgres=# SELECT * FROM pt_trigger;
tg_name | tg_table_name | tg_level  | tg_when | a_old | a_new |
b_old | b_new
+---+---+-+---+---+---+---
 trigger_test21 | pt| STATEMENT | BEFORE  |   |
|   |
 trigger_test43 | pt2   | ROW   | BEFORE  |   | 8
|   | 8
 trigger_test33 | pt2   | ROW   | AFTER   |   | 8
|   | 8
 trigger_test11 | pt| STATEMENT | AFTER   |   |
|   |
(4 rows)

postgres=# TRUNCATE TABLE pt_trigger;
TRUNCATE TABLE
postgres=# UPDATE pt SET a = 2 WHERE a = 1;
UPDATE 1
postgres=# SELECT * FROM pt_trigger;
tg_name | tg_table_name | tg_level  | tg_when | a_old | a_new |
b_old | b_new
+---+---+-+---+---+---+---
 trigger_test21 | pt| STATEMENT | BEFORE  |   |
|   |
 trigger_test22 | pt1   | STATEMENT | BEFORE  |   |
|   |
 trigger_test42 | pt1   | ROW   | BEFORE  | 1 | 2 |
1 | 1
 trigger_test32 | pt1   | ROW   | AFTER   | 1 | 2 |
1 | 1
 trigger_test11 | pt| STATEMENT | AFTER   |   |
|   |
 trigger_test12 | pt1   | STATEMENT | AFTER   |   |
|   |
(6 rows)

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

Hi Rajkumar,

Thanks for testing and the report.

On 2017/04/21 17:00, Rajkumar Raghuwanshi wrote:
> Hi,
> 
> I have observed below with the statement triggers.
> 
> I am able to create statement triggers at root partition, but these
> triggers, not getting fired on updating partition.
> 
> CREATE TABLE pt (a INT, b INT) PARTITION BY RANGE(a);
> CREATE TABLE pt1 PARTITION OF pt FOR VALUES FROM (1) to (7);
> CREATE TABLE pt2 PARTITION OF pt FOR VALUES FROM (7) to (11);
> INSERT INTO pt (a,b) SELECT i,i FROM generate_series(1,10)i;
> CREATE TABLE pt_trigger(TG_NAME varchar,TG_TABLE_NAME varchar,TG_LEVEL
> varchar,TG_WHEN varchar);
> CREATE OR REPLACE FUNCTION process_pt_trigger() RETURNS TRIGGER AS $pttg$
> BEGIN
> IF (TG_OP = 'UPDATE') THEN
> INSERT INTO pt_trigger SELECT
> TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN;
> RETURN NEW;
> END IF;
> RETURN NULL;
> END;
> $pttg$ LANGUAGE plpgsql;
> CREATE TRIGGER pt_trigger_after_p0 AFTER UPDATE ON pt FOR EACH STATEMENT
> EXECUTE PROCEDURE process_pt_trigger();
> CREATE TRIGGER pt_trigger_before_p0 BEFORE UPDATE ON pt FOR EACH STATEMENT
> EXECUTE PROCEDURE process_pt_trigger();
> 
> postgres=# UPDATE pt SET a = 2 WHERE a = 1;
> UPDATE 1
> postgres=# SELECT * FROM pt_trigger ORDER BY 1;
>  tg_name | tg_table_name | tg_level | tg_when
> -+---+--+-
> (0 rows)
> 
> no statement level trigger fired in this case, is this expected behaviour??

I think we discussed this during the development and decided to allow
per-statement triggers on partitioned tables [1], but it seems it doesn't
quite work for update/delete as your example shows.  You can however see
that per-statement triggers of the root partitioned table *are* fired in
the case of insert.

The reason it doesn't work is that we do not allocate ResultRelInfos for
partitioned tables (not even for the root partitioned table in the
update/delete cases), because the current implementation assumes they are
not required.  That's fine only so long as we consider that no rows are
inserted into them, no indexes need to be updated, and that no row-level
triggers are to be fired.  But it misses the fact that we do allow
statement-level triggers on partitioned tables.  So, we should fix things
such that ResultRelInfos are allocated so that any statement-level
triggers are fired.  But there are following questions to consider:

1. Should we consider only the root partitioned table or all partitioned
   tables in a given hierarchy, including the child partitioned tables?
   Note that this is related to a current limitation of updating/deleting
   inheritance hierarchies that we do not currently fire per-statement
   triggers of the child tables.  See the TODO item in wiki:
   https://wiki.postgresql.org/wiki/Todo#Triggers, which says: "When
   statement-level triggers are defined on a parent table, have them fire
   only on the parent table, and fire child table triggers only where
   appropriate"

2. Do we apply the same to inserts on the partitioned tables?  Since
   insert on a partitioned table implicitly affects its partitions, one
   might say that we would need to fire per-statement insert triggers of
   all the partitions.

Meanwhile, attached is a set of patches to fix this.  Descriptions follow:

0001: fire per-statement triggers of the partitioned table mentioned in a
  given update or delete statement

0002: fire per-statement triggers of the child tables during update/delete
  of inheritance hierarchies (including the partitioned table case)

Depending on the answer to 2 above, we can arrange for the per-statement
triggers of all the partitions to be fired upon insert into the
partitioned table.

> but if i am creating triggers on leaf partition, trigger is getting fired.
> 
> CREATE TRIGGER pt_trigger_after_p1 AFTER UPDATE ON pt1 FOR EACH STATEMENT
> EXECUTE PROCEDURE process_pt_trigger();
> CREATE TRIGGER pt_trigger_before_p1 BEFORE UPDATE ON pt1 FOR EACH STATEMENT
> EXECUTE PROCEDURE process_pt_trigger();
> 
> postgres=# UPDATE pt SET a = 5 WHERE a = 4;
> UPDATE 1
> postgres=# SELECT * FROM pt_trigger ORDER BY 1;
>tg_name| tg_table_name | tg_level  | tg_when
> --+---+---+-
>  pt_trigger_after_p1  | pt1   | STATEMENT | AFTER
>  pt_trigger_before_p1 | pt1   | STATEMENT | BEFORE
> (2 rows)

Actually, this works only by accident (with the current implementation,
the *first* partition's triggers will get fired).  If you create another
partition, its per-statement triggers won't get fired.  The patches
described above fixes that.

It would be great if you could check if the patches fix the issues.

Also, adding this to the PostgreSQL 10 open items list.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/8e05817e-14c8-13e4-502b-e440adb24258%40lab.ntt.co.jp
>From 9c7543615ccb05c004591a9176f818413df7ea9d Mon Sep 17 00:00:00 2001
From: amit 

Re: [HACKERS] Declarative partitioning - another take

[Rajkumar Raghuwanshi]

Hi,

I have observed below with the statement triggers.

I am able to create statement triggers at root partition, but these
triggers, not getting fired on updating partition.

CREATE TABLE pt (a INT, b INT) PARTITION BY RANGE(a);
CREATE TABLE pt1 PARTITION OF pt FOR VALUES FROM (1) to (7);
CREATE TABLE pt2 PARTITION OF pt FOR VALUES FROM (7) to (11);
INSERT INTO pt (a,b) SELECT i,i FROM generate_series(1,10)i;
CREATE TABLE pt_trigger(TG_NAME varchar,TG_TABLE_NAME varchar,TG_LEVEL
varchar,TG_WHEN varchar);
CREATE OR REPLACE FUNCTION process_pt_trigger() RETURNS TRIGGER AS $pttg$
BEGIN
IF (TG_OP = 'UPDATE') THEN
INSERT INTO pt_trigger SELECT
TG_NAME,TG_TABLE_NAME,TG_LEVEL,TG_WHEN;
RETURN NEW;
END IF;
RETURN NULL;
END;
$pttg$ LANGUAGE plpgsql;
CREATE TRIGGER pt_trigger_after_p0 AFTER UPDATE ON pt FOR EACH STATEMENT
EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER pt_trigger_before_p0 BEFORE UPDATE ON pt FOR EACH STATEMENT
EXECUTE PROCEDURE process_pt_trigger();

postgres=# UPDATE pt SET a = 2 WHERE a = 1;
UPDATE 1
postgres=# SELECT * FROM pt_trigger ORDER BY 1;
 tg_name | tg_table_name | tg_level | tg_when
-+---+--+-
(0 rows)

no statement level trigger fired in this case, is this expected behaviour??

but if i am creating triggers on leaf partition, trigger is getting fired.

CREATE TRIGGER pt_trigger_after_p1 AFTER UPDATE ON pt1 FOR EACH STATEMENT
EXECUTE PROCEDURE process_pt_trigger();
CREATE TRIGGER pt_trigger_before_p1 BEFORE UPDATE ON pt1 FOR EACH STATEMENT
EXECUTE PROCEDURE process_pt_trigger();

postgres=# UPDATE pt SET a = 5 WHERE a = 4;
UPDATE 1
postgres=# SELECT * FROM pt_trigger ORDER BY 1;
   tg_name| tg_table_name | tg_level  | tg_when
--+---+---+-
 pt_trigger_after_p1  | pt1   | STATEMENT | AFTER
 pt_trigger_before_p1 | pt1   | STATEMENT | BEFORE
(2 rows)

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

Re: [HACKERS] Declarative partitioning vs. information_schema

[Amit Langote]

On 2017/04/14 5:28, Robert Haas wrote:
> On Thu, Apr 6, 2017 at 3:14 AM, Amit Langote
>  wrote:
>>> The bulk of operations that work on traditional tables also work on 
>>> partitions
>>> and partitioned tables.  The next closest kind of relation, a materialized
>>> view, is far less table-like.  Therefore, I recommend showing both 
>>> partitions
>>> and partitioned tables in those views.  This is also consistent with the
>>> decision to use words like "partition" and "partitioned" in messages only 
>>> when
>>> partitioning is relevant to the error.  For example, ATWrongRelkindError()
>>> distinguishes materialized views from tables, but it does not distinguish
>>> tables based on their participation in partitioning.
>>
>> +1
> 
> OK, whoever wants to write the patch, please step forward.

Sorry, perhaps I'm missing something, but I thought there was no patch
left to be written, because the original patch (this thread) implemented
what Noah recommended.

As of HEAD (6cfaffc0ddc):

create table p (a int, b char) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p1a partition of p1 for values in ('a');

\d
   List of relations
 Schema | Name | Type  | Owner
+--+---+---
 public | p| table | amit
 public | p1   | table | amit
 public | p1a  | table | amit
(3 rows)

select tablename from pg_tables where schemaname = 'public';
 tablename
---
 p
 p1
 p1a
(3 rows)

select table_name from information_schema.tables where table_schema =
'public';
 table_name

 p
 p1
 p1a
(3 rows)

Also, it seems that this open item has been listed under Non-bugs, with
remark "firm support for status quo, lack of firm support for alternatives".

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] Declarative partitioning vs. information_schema

[Robert Haas]

On Thu, Apr 6, 2017 at 3:14 AM, Amit Langote
 wrote:
>> The bulk of operations that work on traditional tables also work on 
>> partitions
>> and partitioned tables.  The next closest kind of relation, a materialized
>> view, is far less table-like.  Therefore, I recommend showing both partitions
>> and partitioned tables in those views.  This is also consistent with the
>> decision to use words like "partition" and "partitioned" in messages only 
>> when
>> partitioning is relevant to the error.  For example, ATWrongRelkindError()
>> distinguishes materialized views from tables, but it does not distinguish
>> tables based on their participation in partitioning.
>
> +1

OK, whoever wants to write the patch, please step forward.

/me steps backward.

-- 
Robert Haas
EnterpriseDB: http://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

Re: [HACKERS] Declarative partitioning - another take

[Maksim Milyutin]

On 07.04.2017 13:05, Etsuro Fujita wrote:

On 2016/12/14 16:20, Etsuro Fujita wrote:

On 2016/12/09 19:46, Maksim Milyutin wrote:

I would like to work on two tasks:
 - insert (and eventually update) tuple routing for foreign partition.
 - the ability to create an index on the parent and have all of the
children inherit it;

The first one has been implemented in pg_pathman somehow, but the code
relies on dirty hacks, so the FDW API has to be improved. As for the
extended index support, it doesn't look like a super-hard task.



That would be great!  I'd like to help review the first one.


There seems to be no work on the first one, so I'd like to work on that.


Yes, you can start to work on this, I'll join later as a reviewer.



--
Maksim Milyutin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
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] Declarative partitioning - another take

[Etsuro Fujita]

On 2016/12/14 16:20, Etsuro Fujita wrote:

On 2016/12/09 19:46, Maksim Milyutin wrote:

I would like to work on two tasks:
 - insert (and eventually update) tuple routing for foreign partition.
 - the ability to create an index on the parent and have all of the
children inherit it;

The first one has been implemented in pg_pathman somehow, but the code
relies on dirty hacks, so the FDW API has to be improved. As for the
extended index support, it doesn't look like a super-hard task.



That would be great!  I'd like to help review the first one.


There seems to be no work on the first one, so I'd like to work on that.

Best regards,
Etsuro Fujita




--
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] Declarative partitioning vs. information_schema

[Amit Langote]

On 2017/04/06 16:02, Noah Misch wrote:
> On Wed, Jan 25, 2017 at 01:19:00PM -0500, Robert Haas wrote:
>> On Wed, Jan 25, 2017 at 1:04 PM, Peter Eisentraut
>>  wrote:
>>> On 1/18/17 2:32 PM, Robert Haas wrote:
 Unless we can find something official, I suppose we should just
 display BASE TABLE in that case as we do in other cases.  I wonder if
 the schema needs some broader revision; for example, are there
 information_schema elements intended to show information about
 partitions?
>>>
>>> Is it intentional that we show the partitions by default in \d,
>>> pg_tables, information_schema.tables?  Or should we treat those as
>>> somewhat-hidden details?
>>
>> I'm not really sure what the right thing to do is there.  I was hoping
>> you had an opinion.
> 
> The bulk of operations that work on traditional tables also work on partitions
> and partitioned tables.  The next closest kind of relation, a materialized
> view, is far less table-like.  Therefore, I recommend showing both partitions
> and partitioned tables in those views.  This is also consistent with the
> decision to use words like "partition" and "partitioned" in messages only when
> partitioning is relevant to the error.  For example, ATWrongRelkindError()
> distinguishes materialized views from tables, but it does not distinguish
> tables based on their participation in partitioning.

+1

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] Declarative partitioning vs. information_schema

[Noah Misch]

On Wed, Jan 25, 2017 at 01:19:00PM -0500, Robert Haas wrote:
> On Wed, Jan 25, 2017 at 1:04 PM, Peter Eisentraut
>  wrote:
> > On 1/18/17 2:32 PM, Robert Haas wrote:
> >> Unless we can find something official, I suppose we should just
> >> display BASE TABLE in that case as we do in other cases.  I wonder if
> >> the schema needs some broader revision; for example, are there
> >> information_schema elements intended to show information about
> >> partitions?
> >
> > Is it intentional that we show the partitions by default in \d,
> > pg_tables, information_schema.tables?  Or should we treat those as
> > somewhat-hidden details?
> 
> I'm not really sure what the right thing to do is there.  I was hoping
> you had an opinion.

The bulk of operations that work on traditional tables also work on partitions
and partitioned tables.  The next closest kind of relation, a materialized
view, is far less table-like.  Therefore, I recommend showing both partitions
and partitioned tables in those views.  This is also consistent with the
decision to use words like "partition" and "partitioned" in messages only when
partitioning is relevant to the error.  For example, ATWrongRelkindError()
distinguishes materialized views from tables, but it does not distinguish
tables based on their participation in partitioning.


-- 
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] Declarative partitioning vs. information_schema

[Amit Langote]

On 2017/01/26 3:19, Robert Haas wrote:
> On Wed, Jan 25, 2017 at 1:04 PM, Peter Eisentraut
>  wrote:
>> On 1/18/17 2:32 PM, Robert Haas wrote:
>>> Unless we can find something official, I suppose we should just
>>> display BASE TABLE in that case as we do in other cases.  I wonder if
>>> the schema needs some broader revision; for example, are there
>>> information_schema elements intended to show information about
>>> partitions?
>>
>> Is it intentional that we show the partitions by default in \d,
>> pg_tables, information_schema.tables?  Or should we treat those as
>> somewhat-hidden details?
> 
> I'm not really sure what the right thing to do is there.  I was hoping
> you had an opinion.

I guess this is an open item then.  I think Greg Stark brought this up too
on the original partitioning thread [1].

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAM-w4HOZ5fPS7GoCTTrW42q01%2BwPrOWFCnr9H0iDyVTZP2H1CA%40mail.gmail.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] Declarative partitioning optimization for large amount of partitions

[Amit Langote]

On Fri, Mar 24, 2017 at 11:06 PM, Simon Riggs  wrote:
> On 1 March 2017 at 01:36, Amit Langote  wrote:
>
>> I don't know which way you're thinking of fixing this, but a planner patch
>> to implement faster partition-pruning will have taken care of this, I
>> think.  As you may know, even declarative partitioned tables currently
>> depend on constraint exclusion for partition-pruning and planner's current
>> approach of handling inheritance requires to open all the child tables
>> (partitions), whereas the new approach hopefully shouldn't need to do
>> that.  I am not sure if looking for a more localized fix for this would be
>> worthwhile, although I may be wrong.
>
> What "new approach" are we discussing?
>
> Is there a patch or design discussion?

Neither at the moment.  As Aleksander said in his reply I was
referring to Dmitry Ivanov's plan of porting pg_pathman's planner
functionality to core that he mentioned on the declarative
partitioning thread back in December [1].

Thanks,
Amit

[1] 
https://www.postgresql.org/message-id/426b2b01-61e0-43aa-bd84-c6fcf516f1c3%40postgrespro.ru


-- 
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] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Simon,

> > I don't know which way you're thinking of fixing this, but a planner patch
> > to implement faster partition-pruning will have taken care of this, I
> > think.  As you may know, even declarative partitioned tables currently
> > depend on constraint exclusion for partition-pruning and planner's current
> > approach of handling inheritance requires to open all the child tables
> > (partitions), whereas the new approach hopefully shouldn't need to do
> > that.  I am not sure if looking for a more localized fix for this would be
> > worthwhile, although I may be wrong.
> 
> What "new approach" are we discussing?
> Is there a patch or design discussion?

I think what was meant was plans of my colleague Dmitry Ivanov to
implement partition-pruning. I've just spoke with Dmitry about this
matter. Unless there is anyone else who is already working on this
optimization we would like to work on it together. Unfortunately there
is no patch or design discussion of partition-pruning on this
commitfest.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Simon Riggs]

On 1 March 2017 at 01:36, Amit Langote  wrote:

> I don't know which way you're thinking of fixing this, but a planner patch
> to implement faster partition-pruning will have taken care of this, I
> think.  As you may know, even declarative partitioned tables currently
> depend on constraint exclusion for partition-pruning and planner's current
> approach of handling inheritance requires to open all the child tables
> (partitions), whereas the new approach hopefully shouldn't need to do
> that.  I am not sure if looking for a more localized fix for this would be
> worthwhile, although I may be wrong.

What "new approach" are we discussing?

Is there a patch or design discussion?

-- 
Simon Riggshttp://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] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Tels,

Thanks a lot for the review!

> "corresponding"

Fixed.

> Also a question: Some one-line comments are
> 
>  /* Comment. */
> 
> while others are
> 
>  /*
>   * Comment.
>   */
> 
> Why the difference?

I'm trying to follow a code stile used in a code I'm modifying. In this
case I got an impression that second style of one-line comments is used
more often an I tried to follow it but apparently I didn't succeed :)
This is fixed as well.

On Thu, Mar 09, 2017 at 06:40:39PM -0500, Tels wrote:
> Hi Aleksander,
> 
> noticed this in your patch:
> 
> > * Add a corespinding entry to pgStatTabHash.
> 
> "corresponding"
> 
> Also a question: Some one-line comments are
> 
>  /* Comment. */
> 
> while others are
> 
>  /*
>   * Comment.
>   */
> 
> Why the difference?
> 
> Hope this helps,
> 
> Tels

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7cacb1e9b2..a22a5a37c8 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -161,6 +161,20 @@ typedef struct TabStatusArray
 static TabStatusArray *pgStatTabList = NULL;
 
 /*
+ * pgStatTabHash entry
+ */
+typedef struct TabStatHashEntry
+{
+	Oid t_id;
+	PgStat_TableStatus* tsa_entry;
+} TabStatHashEntry;
+
+/*
+ * Hash table for O(1) t_id -> tsa_entry lookup
+ */
+static HTAB *pgStatTabHash = NULL;
+
+/*
  * Backends store per-function info that's waiting to be sent to the collector
  * in this hash table (indexed by function OID).
  */
@@ -815,7 +829,13 @@ pgstat_report_stat(bool force)
 pgstat_send_tabstat(this_msg);
 this_msg->m_nentries = 0;
 			}
+
+			/*
+			 * Entry will be zeroed soon so we need to remove it from the lookup table.
+			 */
+			hash_search(pgStatTabHash, >t_id, HASH_REMOVE, NULL);
 		}
+
 		/* zero out TableStatus structs after use */
 		MemSet(tsa->tsa_entries, 0,
 			   tsa->tsa_used * sizeof(PgStat_TableStatus));
@@ -1667,59 +1687,77 @@ pgstat_initstats(Relation rel)
 }
 
 /*
+ * Make sure pgStatTabList and pgStatTabHash are initialized.
+ */
+static void
+make_sure_stat_tab_initialized()
+{
+	HASHCTL ctl;
+
+	if (pgStatTabList)
+		return;
+
+	Assert(!pgStatTabHash);
+
+	memset(, 0, sizeof(ctl));
+	ctl.keysize = sizeof(Oid);
+	ctl.entrysize = sizeof(TabStatHashEntry);
+	ctl.hcxt = TopMemoryContext;
+
+	pgStatTabHash = hash_create("pgstat t_id to tsa_entry lookup table",
+		TABSTAT_QUANTUM, , HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	pgStatTabList = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+sizeof(TabStatusArray));
+}
+
+/*
  * get_tabstat_entry - find or create a PgStat_TableStatus entry for rel
  */
 static PgStat_TableStatus *
 get_tabstat_entry(Oid rel_id, bool isshared)
 {
+	TabStatHashEntry* hash_entry;
 	PgStat_TableStatus *entry;
 	TabStatusArray *tsa;
-	TabStatusArray *prev_tsa;
-	int			i;
+	bool found;
+
+	make_sure_stat_tab_initialized();
+
+	/*
+	 * Find an entry or create a new one.
+	 */
+	hash_entry = hash_search(pgStatTabHash, _id, HASH_ENTER, );
+	if(found)
+		return hash_entry->tsa_entry;
 
 	/*
-	 * Search the already-used tabstat slots for this relation.
+	 * `hash_entry` was just created and now we have to fill it.
+	 * First make sure there is a free space in a last element of pgStatTabList.
 	 */
-	prev_tsa = NULL;
-	for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = tsa->tsa_next)
+	tsa = pgStatTabList;
+	while(tsa->tsa_used == TABSTAT_QUANTUM)
 	{
-		for (i = 0; i < tsa->tsa_used; i++)
+		if(tsa->tsa_next == NULL)
 		{
-			entry = >tsa_entries[i];
-			if (entry->t_id == rel_id)
-return entry;
+			tsa->tsa_next = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+		sizeof(TabStatusArray));
 		}
 
-		if (tsa->tsa_used < TABSTAT_QUANTUM)
-		{
-			/*
-			 * It must not be present, but we found a free slot instead. Fine,
-			 * let's use this one.  We assume the entry was already zeroed,
-			 * either at creation or after last use.
-			 */
-			entry = >tsa_entries[tsa->tsa_used++];
-			entry->t_id = rel_id;
-			entry->t_shared = isshared;
-			return entry;
-		}
+		tsa = tsa->tsa_next;
 	}
 
 	/*
-	 * We ran out of tabstat slots, so allocate more.  Be sure they're zeroed.
-	 */
-	tsa = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
-	sizeof(TabStatusArray));
-	if (prev_tsa)
-		prev_tsa->tsa_next = tsa;
-	else
-		pgStatTabList = tsa;
-
-	/*
-	 * Use the first entry of the new TabStatusArray.
+	 * Add an entry.
 	 */
 	entry = >tsa_entries[tsa->tsa_used++];
 	entry->t_id = rel_id;
 	entry->t_shared = isshared;
+
+	/*
+	 * Add a corresponding entry to pgStatTabHash.
+	 */
+	hash_entry->tsa_entry = entry;
 	return entry;
 }
 
@@ -1731,22 +1769,19 @@ get_tabstat_entry(Oid rel_id, bool isshared)
 PgStat_TableStatus *
 find_tabstat_entry(Oid rel_id)
 {
-	PgStat_TableStatus *entry;
-	TabStatusArray *tsa;
-	int			i;
+	TabStatHashEntry* hash_entry;
 
-	for (tsa = pgStatTabList; tsa != NULL; tsa = tsa->tsa_next)
-	{
-		

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Tels]

Hi Aleksander,

noticed this in your patch:

> * Add a corespinding entry to pgStatTabHash.

"corresponding"

Also a question: Some one-line comments are

 /* Comment. */

while others are

 /*
  * Comment.
  */

Why the difference?

Hope this helps,

Tels

PS: Sorry if this appears twice, I fumbled the From: ...



-- 
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] Declarative partitioning optimization for large amount of partitions

[Tels]

Hi Aleksander,

noticed this in your patch:

> * Add a corespinding entry to pgStatTabHash.

"corresponding"

Also a question: Some one-line comments are

 /* Comment. */

while others are

 /*
  * Comment.
  */

Why the difference?

Hope this helps,

Tels


-- 
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] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Amit,

> Sorry, I didn't mean to dissuade you from trying those
> micro-optimizations.  If general inheritance cases can benefit from it
> (which, until we have a different method, will be used by partitioned
> tables as well), I think we should try it.

OK, I'll see what could be done here as well then.

On Tue, Mar 07, 2017 at 10:55:12AM +0900, Amit Langote wrote:
> Hi Aleksander,
> 
> On 2017/03/07 0:22, Aleksander Alekseev wrote:
> > Hello.
> > 
> > OK, here is a patch.
> > 
> > Benchmark, before:
> > 
> > ```
> > number of transactions actually processed: 1823
> > latency average = 1153.495 ms
> > latency stddev = 154.366 ms
> > tps = 6.061104 (including connections establishing)
> > tps = 6.061211 (excluding connections establishing)
> > ```
> > 
> > Benchmark, after:
> > 
> > ```
> > number of transactions actually processed: 2462
> > latency average = 853.862 ms
> > latency stddev = 112.270 ms
> > tps = 8.191861 (including connections establishing)
> > tps = 8.192028 (excluding connections establishing)
> > ```
> > 
> > +35% TPS, just as expected. Feel free to run your own benchmarks on
> > different datasets and workloads. `perf top` shows that first bottleneck
> > is completely eliminated.
> 
> That seems like a good gain.
> 
> > I did nothing about the second bottleneck
> > since as Amit mentioned partition-pruning should solve this anyway and
> > apparently any micro-optimizations don't worth an effort.
> 
> Sorry, I didn't mean to dissuade you from trying those
> micro-optimizations.  If general inheritance cases can benefit from it
> (which, until we have a different method, will be used by partitioned
> tables as well), I think we should try it.
> 
> Thanks,
> Amit
> 
> 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi, Andres

Thanks a lot for the review!

> Why are we keeping that list / the "batch" allocation pattern? It
> doesn't actually seem useful to me after these changes.  Given that we
> don't shrink hash-tables, we could just use the hash-table memory for
> this, no?

I don't think we can do that. According to comments:

```
 * NOTE: once allocated, TabStatusArray structures are never moved or deleted
 [...]
 * This allows relcache pgstat_info pointers to be treated as long-lived data,
 * avoiding repeated searches in pgstat_initstats() when a relation is
 * repeatedly opened during a transaction.
```

It is my understanding that dynahash can't give same guarantees.

> 'faster ... lookup' doesn't strike me as a useful comment, because it's
> only useful in relation of the current code to the new code.

Agree, fixed to 'O(1) ... lookup'.

> It's not really freed...

Right, it's actually zeroed. Fixed.

> Arguably it'd be better to HASH_ENTER directly here, instead of doing
> two lookups.

Good point. Fixed.

> Think it'd be better to move the hash creation into its own function.

Agree, fixed.

On Mon, Mar 06, 2017 at 12:01:50PM -0800, Andres Freund wrote:
> Hi,
> 
> This issue has bothered me in non-partitioned use-cases recently, so
> thanks for taking it up.
> 
> 
> On 2017-03-06 18:22:17 +0300, Aleksander Alekseev wrote:
> > diff --git a/src/backend/postmaster/pgstat.c 
> > b/src/backend/postmaster/pgstat.c
> > index 2fb9a8bf58..fa906e7930 100644
> > --- a/src/backend/postmaster/pgstat.c
> > +++ b/src/backend/postmaster/pgstat.c
> > @@ -160,6 +160,16 @@ typedef struct TabStatusArray
> >  
> >  static TabStatusArray *pgStatTabList = NULL;
> 
> Why are we keeping that list / the "batch" allocation pattern? It
> doesn't actually seem useful to me after these changes.  Given that we
> don't shrink hash-tables, we could just use the hash-table memory for
> this, no?
> 
> I think a separate list that only contains things that are "active" in
> the current transaction makes sense, because the current logic requires
> iterating over a potentially very large array at transaction commit.
> 
> 
> > +/* pgStatTabHash entry */
> > +typedef struct TabStatHashEntry
> > +{
> > +   Oid t_id;
> > +   PgStat_TableStatus* tsa_entry;
> > +} TabStatHashEntry;
> > +
> > +/* Hash table for faster t_id -> tsa_entry lookup */
> > +static HTAB *pgStatTabHash = NULL;
> > +
> 
> 'faster ... lookup' doesn't strike me as a useful comment, because it's
> only useful in relation of the current code to the new code.
> 
> 
> 
> >  /*
> >   * Backends store per-function info that's waiting to be sent to the 
> > collector
> >   * in this hash table (indexed by function OID).
> > @@ -815,7 +825,13 @@ pgstat_report_stat(bool force)
> > pgstat_send_tabstat(this_msg);
> > this_msg->m_nentries = 0;
> > }
> > +
> > +   /*
> > +* Entry will be freed soon so we need to remove it 
> > from the lookup table.
> > +*/
> > +   hash_search(pgStatTabHash, >t_id, HASH_REMOVE, 
> > NULL);
> > }
> 
> It's not really freed...
> 
> 
> >  static PgStat_TableStatus *
> >  get_tabstat_entry(Oid rel_id, bool isshared)
> >  {
> > +   TabStatHashEntry* hash_entry;
> > PgStat_TableStatus *entry;
> > TabStatusArray *tsa;
> > -   TabStatusArray *prev_tsa;
> > -   int i;
> > +
> > +   /* Try to find an entry */
> > +   entry = find_tabstat_entry(rel_id);
> > +   if(entry != NULL)
> > +   return entry;
> 
> Arguably it'd be better to HASH_ENTER directly here, instead of doing
> two lookups.
> 
> 
> > /*
> > -* Search the already-used tabstat slots for this relation.
> > +* Entry doesn't exist - creating one.
> > +* First make sure there is a free space in a last element of 
> > pgStatTabList.
> >  */
> > -   prev_tsa = NULL;
> > -   for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = 
> > tsa->tsa_next)
> > +   if (!pgStatTabList)
> > {
> > -   for (i = 0; i < tsa->tsa_used; i++)
> > -   {
> > -   entry = >tsa_entries[i];
> > -   if (entry->t_id == rel_id)
> > -   return entry;
> > -   }
> > +   HASHCTL ctl;
> >  
> > -   if (tsa->tsa_used < TABSTAT_QUANTUM)
> > +   Assert(!pgStatTabHash);
> > +
> > +   memset(, 0, sizeof(ctl));
> > +   ctl.keysize = sizeof(Oid);
> > +   ctl.entrysize = sizeof(TabStatHashEntry);
> > +   ctl.hcxt = TopMemoryContext;
> > +
> > +   pgStatTabHash = hash_create("pgstat t_id to tsa_entry lookup 
> > table",
> > +   TABSTAT_QUANTUM, , HASH_ELEM | HASH_BLOBS | 
> > HASH_CONTEXT);
> 
> Think it'd be better to move the hash creation into its own function.
> 
> 
> - Andres

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c 

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Amit Langote]

Hi Aleksander,

On 2017/03/07 0:22, Aleksander Alekseev wrote:
> Hello.
> 
> OK, here is a patch.
> 
> Benchmark, before:
> 
> ```
> number of transactions actually processed: 1823
> latency average = 1153.495 ms
> latency stddev = 154.366 ms
> tps = 6.061104 (including connections establishing)
> tps = 6.061211 (excluding connections establishing)
> ```
> 
> Benchmark, after:
> 
> ```
> number of transactions actually processed: 2462
> latency average = 853.862 ms
> latency stddev = 112.270 ms
> tps = 8.191861 (including connections establishing)
> tps = 8.192028 (excluding connections establishing)
> ```
> 
> +35% TPS, just as expected. Feel free to run your own benchmarks on
> different datasets and workloads. `perf top` shows that first bottleneck
> is completely eliminated.

That seems like a good gain.

> I did nothing about the second bottleneck
> since as Amit mentioned partition-pruning should solve this anyway and
> apparently any micro-optimizations don't worth an effort.

Sorry, I didn't mean to dissuade you from trying those
micro-optimizations.  If general inheritance cases can benefit from it
(which, until we have a different method, will be used by partitioned
tables as well), I think we should try it.

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] Declarative partitioning optimization for large amount of partitions

[Andres Freund]

Hi,

This issue has bothered me in non-partitioned use-cases recently, so
thanks for taking it up.


On 2017-03-06 18:22:17 +0300, Aleksander Alekseev wrote:
> diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
> index 2fb9a8bf58..fa906e7930 100644
> --- a/src/backend/postmaster/pgstat.c
> +++ b/src/backend/postmaster/pgstat.c
> @@ -160,6 +160,16 @@ typedef struct TabStatusArray
>  
>  static TabStatusArray *pgStatTabList = NULL;

Why are we keeping that list / the "batch" allocation pattern? It
doesn't actually seem useful to me after these changes.  Given that we
don't shrink hash-tables, we could just use the hash-table memory for
this, no?

I think a separate list that only contains things that are "active" in
the current transaction makes sense, because the current logic requires
iterating over a potentially very large array at transaction commit.


> +/* pgStatTabHash entry */
> +typedef struct TabStatHashEntry
> +{
> + Oid t_id;
> + PgStat_TableStatus* tsa_entry;
> +} TabStatHashEntry;
> +
> +/* Hash table for faster t_id -> tsa_entry lookup */
> +static HTAB *pgStatTabHash = NULL;
> +

'faster ... lookup' doesn't strike me as a useful comment, because it's
only useful in relation of the current code to the new code.



>  /*
>   * Backends store per-function info that's waiting to be sent to the 
> collector
>   * in this hash table (indexed by function OID).
> @@ -815,7 +825,13 @@ pgstat_report_stat(bool force)
>   pgstat_send_tabstat(this_msg);
>   this_msg->m_nentries = 0;
>   }
> +
> + /*
> +  * Entry will be freed soon so we need to remove it 
> from the lookup table.
> +  */
> + hash_search(pgStatTabHash, >t_id, HASH_REMOVE, 
> NULL);
>   }

It's not really freed...


>  static PgStat_TableStatus *
>  get_tabstat_entry(Oid rel_id, bool isshared)
>  {
> + TabStatHashEntry* hash_entry;
>   PgStat_TableStatus *entry;
>   TabStatusArray *tsa;
> - TabStatusArray *prev_tsa;
> - int i;
> +
> + /* Try to find an entry */
> + entry = find_tabstat_entry(rel_id);
> + if(entry != NULL)
> + return entry;

Arguably it'd be better to HASH_ENTER directly here, instead of doing
two lookups.


>   /*
> -  * Search the already-used tabstat slots for this relation.
> +  * Entry doesn't exist - creating one.
> +  * First make sure there is a free space in a last element of 
> pgStatTabList.
>*/
> - prev_tsa = NULL;
> - for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = 
> tsa->tsa_next)
> + if (!pgStatTabList)
>   {
> - for (i = 0; i < tsa->tsa_used; i++)
> - {
> - entry = >tsa_entries[i];
> - if (entry->t_id == rel_id)
> - return entry;
> - }
> + HASHCTL ctl;
>  
> - if (tsa->tsa_used < TABSTAT_QUANTUM)
> + Assert(!pgStatTabHash);
> +
> + memset(, 0, sizeof(ctl));
> + ctl.keysize = sizeof(Oid);
> + ctl.entrysize = sizeof(TabStatHashEntry);
> + ctl.hcxt = TopMemoryContext;
> +
> + pgStatTabHash = hash_create("pgstat t_id to tsa_entry lookup 
> table",
> + TABSTAT_QUANTUM, , HASH_ELEM | HASH_BLOBS | 
> HASH_CONTEXT);

Think it'd be better to move the hash creation into its own function.


- Andres


-- 
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] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hello.

OK, here is a patch.

Benchmark, before:

```
number of transactions actually processed: 1823
latency average = 1153.495 ms
latency stddev = 154.366 ms
tps = 6.061104 (including connections establishing)
tps = 6.061211 (excluding connections establishing)
```

Benchmark, after:

```
number of transactions actually processed: 2462
latency average = 853.862 ms
latency stddev = 112.270 ms
tps = 8.191861 (including connections establishing)
tps = 8.192028 (excluding connections establishing)
```

+35% TPS, just as expected. Feel free to run your own benchmarks on
different datasets and workloads. `perf top` shows that first bottleneck
is completely eliminated. I did nothing about the second bottleneck
since as Amit mentioned partition-pruning should solve this anyway and
apparently any micro-optimizations don't worth an effort.

Also I checked test coverage using lcov to make sure that this code is
test covered. An exact script I'm using could be found here [1].

[1] https://github.com/afiskon/pgscripts/blob/master/code-coverage.sh

On Wed, Mar 01, 2017 at 10:36:24AM +0900, Amit Langote wrote:
> Hi,
> 
> On 2017/02/28 23:25, Aleksander Alekseev wrote:
> > Hello.
> > 
> > I decided to figure out whether current implementation of declarative
> > partitioning has any bottlenecks when there is a lot of partitions. Here
> > is what I did [1].
> 
> Thanks for sharing.
> 
> > Then:
> > 
> > ```
> > # 2580 is some pk that exists
> > echo 'select * from part_test where pk = 2580;' > t.sql
> > pgbench -j 7 -c 7 -f t.sql -P 1 -T 300 eax
> > ```
> > 
> > `perf top` showed to bottlenecks [2]. A stacktrace for the first one
> > looks like this [3]:
> > 
> > ```
> > 0x007a42e2 in get_tabstat_entry (rel_id=25696, isshared=0 '\000') 
> > at pgstat.c:1689
> > 1689if (entry->t_id == rel_id)
> > #0  0x007a42e2 in get_tabstat_entry (rel_id=25696, isshared=0 
> > '\000') at pgstat.c:1689
> > #1  0x007a4275 in pgstat_initstats (rel=0x7f4af3fd41f8) at 
> > pgstat.c:1666
> > #2  0x004c7090 in relation_open (relationId=25696, lockmode=0) at 
> > heapam.c:1137
> > #3  0x004c72c9 in heap_open (relationId=25696, lockmode=0) at 
> > heapam.c:1291
> > (skipped)
> > ```
> > 
> > And here is a stacktrace for the second bottleneck [4]:
> > 
> > ```
> > 0x00584fb1 in find_all_inheritors (parentrelId=16393, lockmode=1, 
> > numparents=0x0) at pg_inherits.c:199
> > 199 forboth(lo, rels_list, li, rel_numparents)
> > #0  0x00584fb1 in find_all_inheritors (parentrelId=16393, 
> > lockmode=1, numparents=0x0) at pg_inherits.c:199
> > #1  0x0077fc9f in expand_inherited_rtentry (root=0x1badcb8, 
> > rte=0x1b630b8, rti=1) at prepunion.c:1408
> > #2  0x0077fb67 in expand_inherited_tables (root=0x1badcb8) at 
> > prepunion.c:1335
> > #3  0x00767526 in subquery_planner (glob=0x1b63cc0, 
> > parse=0x1b62fa0, parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) 
> > at planner.c:568
> > (skipped)
> > ```
> > 
> > The first one could be easily fixed by introducing a hash table
> > (rel_id -> pgStatList entry). Perhaps hash table should be used only
> > after some threshold. Unless there are any objections I will send a
> > corresponding patch shortly.
> 
> I have never thought about this one really.
> 
> > I didn't explored the second bottleneck closely yet but at first glance
> > it doesn't look much more complicated.
> 
> I don't know which way you're thinking of fixing this, but a planner patch
> to implement faster partition-pruning will have taken care of this, I
> think.  As you may know, even declarative partitioned tables currently
> depend on constraint exclusion for partition-pruning and planner's current
> approach of handling inheritance requires to open all the child tables
> (partitions), whereas the new approach hopefully shouldn't need to do
> that.  I am not sure if looking for a more localized fix for this would be
> worthwhile, although I may be wrong.
> 
> Thanks,
> Amit
> 
> 

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 2fb9a8bf58..fa906e7930 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -160,6 +160,16 @@ typedef struct TabStatusArray
 
 static TabStatusArray *pgStatTabList = NULL;
 
+/* pgStatTabHash entry */
+typedef struct TabStatHashEntry
+{
+	Oid t_id;
+	PgStat_TableStatus* tsa_entry;
+} TabStatHashEntry;
+
+/* Hash table for faster t_id -> tsa_entry lookup */
+static HTAB *pgStatTabHash = NULL;
+
 /*
  * Backends store per-function info that's waiting to be sent to the collector
  * in this hash table (indexed by function OID).
@@ -815,7 +825,13 @@ pgstat_report_stat(bool force)
 pgstat_send_tabstat(this_msg);
 this_msg->m_nentries = 0;
 			}
+
+			/*
+			 * Entry will be freed soon so we need to remove it from the lookup table.
+			 */
+