On 2017/06/03 1:31, Robert Haas wrote:
> If you create a partitioned table in the obvious way, partcollation ends up 0:
> 
> rhaas=# create table foo (a int, b text) partition by list (a);
> CREATE TABLE
> rhaas=# select * from pg_partitioned_table;
>  partrelid | partstrat | partnatts | partattrs | partclass |
> partcollation | partexprs
> -----------+-----------+-----------+-----------+-----------+---------------+-----------
>      16420 | l         |         1 | 1         | 1978      | 0             |
> (1 row)
> 
> You could argue that 0 is an OK value there; offhand, I'm not sure
> about that.

If you create index on an integer column, you'll get a 0 in indcollation:

select indcollation from pg_index where indrelid = 'foo'::regclass;
 indcollation
--------------
 0
(1 row)


> But there's nothing in
> https://www.postgresql.org/docs/10/static/catalog-pg-partitioned-table.html
> which indicates that some entries can be 0 rather than a valid
> collation OID.

Yeah, it might be better to explain that.  BTW, pg_index.indcollation's
description lacks a note about this too, so maybe, we should fix both.
OTOH, pg_attribute.attcollation's description mentions it:

       The defined collation of the column, or zero if the column is
       not of a collatable data type.

It might be a good idea to update partcollation's and indcollation's
description along the same lines.

> And this is definitely not OK:
> 
> rhaas=# select * from pg_depend where objid = 16420;
>  classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
> ---------+-------+----------+------------+----------+-------------+---------
>     1259 | 16420 |        0 |       2615 |     2200 |           0 | n
>     1259 | 16420 |        0 |       3456 |        0 |           0 | n
> (2 rows)
> 
> We shouldn't be storing a dependency on non-existing collation 0.
>
> I'm not sure whether the bug here is that we should have a valid
> collation OID there rather than 0, or whether the bug is that we
> shouldn't be recording a dependency on anything other than a real
> collation OID, but something about this is definitely not right.

I think we can call it a bug of StorePartitionKey().  I looked at the
similar code in index_create() (which actually I had originally looked at
for reference when writing the partitioning code in question) and looks
like it doesn't store the dependency for collation 0 and for the default
collation of the database.  I think the partitioning code should do the
same.  Attached find a patch for the same (which also updates the
documentation as mentioned above); with the patch:

create table p (a int) partition by range (a);
select partcollation from pg_partitioned_table;
 partcollation
---------------
 0
(1 row)


-- no collation dependency stored for invalid collation
select * from pg_depend where objid = 'p'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
    1259 | 16434 |        0 |       2615 |     2200 |           0 | n
(1 row)


create table p (a text) partition by range (a);
select partcollation from pg_partitioned_table;
 partcollation
---------------
 100
(1 row)

-- no collation dependency stored for the default collation
select * from pg_depend where objid = 'p'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
    1259 | 16407 |        0 |       2615 |     2200 |           0 | n
(1 row)


create table p (a text) partition by range (a collate "en_US");
select partcollation from pg_partitioned_table;
 partcollation
---------------
 12513
(1 row)

-- dependency on non-default collation is stored
select * from pg_depend where objid = 'p'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
    1259 | 16410 |        0 |       2615 |     2200 |           0 | n
    1259 | 16410 |        0 |       3456 |    12513 |           0 | n
(2 rows)


BTW, the places which check whether the collation to store a dependency
for is the database default collation don't need to do that.  I mean the
following code block in all of these places:

       /* The default collation is pinned, so don't bother recording it */
       if (OidIsValid(attr->attcollation) &&
           attr->attcollation != DEFAULT_COLLATION_OID)
       {
           referenced.classId = CollationRelationId;
           referenced.objectId = attr->attcollation;
           referenced.objectSubId = 0;
           recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
       }

That's because the default collation is pinned and the dependency code
checks isObjectPinned() and does not create pg_depend entries if so.
Those places are:

AddNewAttributeTuples
StorePartitionKey
index_create
GenerateTypeDependencies
add_column_collation_dependency

Removing that check still passes `make check-world`.

Thanks,
Amit
From 9c7caf3b762f8c7a70fa02b867427c5c33696b4b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 5 Jun 2017 10:58:52 +0900
Subject: [PATCH] Do not store dependency on invalid and default collation

Partitioning code failed to comply with that rule.
---
 doc/src/sgml/catalogs.sgml |  6 ++++--
 src/backend/catalog/heap.c | 11 ++++++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b2fae027f5..7decc5a91e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3783,7 +3783,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration 
count&gt;</>:<replaceable>&lt;salt&gt;<
       <entry><literal><link 
linkend="catalog-pg-collation"><structname>pg_collation</structname></link>.oid</literal></entry>
       <entry>
        For each column in the index key, this contains the OID of the
-       collation to use for the index.
+       the collation to use for the index, or zero if the column is not
+       of a collatable data type.
       </entry>
      </row>
 
@@ -4770,7 +4771,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration 
count&gt;</>:<replaceable>&lt;salt&gt;<
       <entry><literal><link 
linkend="catalog-pg-opclass"><structname>pg_opclass</structname></link>.oid</literal></entry>
       <entry>
        For each column in the partition key, this contains the OID of the
-       the collation to use for partitioning.
+       the collation to use for partitioning, or zero if the column is not
+       of a collatable data type.
       </entry>
      </row>
 
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 0ce94f346f..4e5b79ef94 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3160,9 +3160,14 @@ StorePartitionKey(Relation rel,
 
                recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
 
-               referenced.classId = CollationRelationId;
-               referenced.objectId = partcollation[i];
-               referenced.objectSubId = 0;
+               /* The default collation is pinned, so don't bother recording 
it */
+               if (OidIsValid(partcollation[i]) &&
+                       partcollation[i] != DEFAULT_COLLATION_OID)
+               {
+                       referenced.classId = CollationRelationId;
+                       referenced.objectId = partcollation[i];
+                       referenced.objectSubId = 0;
+               }
 
                recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
        }
-- 
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

Reply via email to