Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 20 June 2017 at 03:42, Thomas Munro  wrote:
> Just a thought: If I understand correctly this new array of tuple
> conversion maps is the same as mtstate->mt_transition_tupconv_maps in
> my patch transition-tuples-from-child-tables-v11.patch (hopefully soon
> to be committed to close a PG10 open item).  In my patch I bounce
> transition tuples from child relations up to the named relation's
> triggers, and in this patch you bounce child tuples up to the named
> relation for rerouting, so the conversion requirement is the same.
> Perhaps we could consider refactoring to build a common struct member
> on demand for the row movement patch at some point in the future if it
> makes the code cleaner.

I agree; thanks for bringing this to my attention. The conversion maps
in my patch and yours do sound like they are exactly same. And even in
case where both update-row-movement and transition tables are playing
together, the same map should serve the purpose of both. I will keep a
watch on your patch, and check how I can adjust my patch so that I
don't have to refactor the mapping.

One difference I see is : in your patch, in ExecModifyTable() we jump
the current map position for each successive subplan, whereas in my
patch, in ExecInsert() we deduce the position of the right map to be
fetched using the position of the current resultRelInfo in the
mtstate->resultRelInfo[] array. I think your way is more consistent
with the existing code.

-- 
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] ECPG: WHENEVER statement with DO CONTINUE action

[vinayak]

On 2017/06/12 13:09, vinayak wrote:


Hi,

On 2017/06/10 12:23, Vinayak Pokale wrote:


Thank you for your reply

On Jun 9, 2017 5:39 PM, "Michael Meskes" > wrote:

>
> Could you please add a "DO CONTINUE" case to one of the test cases? Or
> add a new one? We would need a test case IMO.
>
Yes I will add test case and send updated patch.


I have added new test case for DO CONTINUE.
Please check the attached patch.


I have added this in Sept. CF
https://commitfest.postgresql.org/14/1173/

Regards,
Vinayak Pokale
NTT Open Source Software Center

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Surafel,

>Your patch doesn't cover security labels on databases which have similar
issue
>and consider dividing the patch into two one for adding CURRENT_DATABASE
as a
>database specifier and the other for adding database-level information to
pg_dump output
>in a way that allows to load a dump into a differently named database.

Thanks your review and suggestion. I will add them.


Regards,
Jing Wang
Fujitsu Australia

Re: [HACKERS] REPLICA IDENTITY FULL

[Andres Freund]

On 2017-06-20 11:52:10 +0900, Tatsuo Ishii wrote:
> If my understanding is correct, it would not be easy to fix, no?
> 
> > We might be able to refine that, but there is a general problem that
> > without an index and an operator class, we are just doing our random
> > best to match the values.

I don't see the problem as being big.  We should just look up the
default btree opclass and use the relevant operator.  That's a how a
number of things already work.

- 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: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
 wrote:
> As far as I can see, there are a couple of things that I still need to
> work on to make people happy:
> - Rework the generic APIs for TLS finish and endpoint so as any
> implementation can use channel binding without inducing any extra code
> footprint to be-secure.c and fe-secure.c.
> - Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
> - Have a couple of tests for channel binding to allow people to test
> the feature easily. Those will be in src/test/ssl/. It would be nice
> as well to be able to enforce the channel binding type on libpq-side,
> which is useful at least for testing. So we are going to need an
> environment variable for this purpose, and a connection parameter.

Okay, here we go. Attached is a set of four patches:
- 0001 is some refactoring for the SSL tests so as other test suite in
src/test/ssl can take advantage of the connection routines. There is
nothing fancy here.
- 0002 is the implementation of tls-unique as channel binding. This
has been largely reworked since last submission, I have found on the
way a couple of bugs and some correctness issues.
- 0003 is a patch to add as connection parameters saslname and
saslchannelbinding. With support of more SASL mechanisms (PG10 has
SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
used to enforce on the client-side the value of the SASL mechanism
chosen. saslchannelbinding does the same for the channel binding name.
This is very useful for testing, and a set of tests are added in
src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
many scenarios, like downgrade attacks for example.
- 0004 is the implementation of tls-server-end-point, as Alvaro has
asked. Per RFC 5929, the binding data needs to be a hash of the server
certificate. If the signature algorithm of the certificate is MD5 or
SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
are used to hash the data. The hashed data is then encoded in base64
and sent to the server for verification. Tests using saslchannelname
have been added as well. It took me a while to find out that
OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
find out the algorithm of a certificate with OpenSSL.

With the tests directly in the patch, things are easy to run. WIth
PG10 stabilization work, of course I don't expect much feedback :)
But this set of patches looks like the direction we want to go so as
JDBC and libpq users can take advantage of channel binding with SCRAM.
-- 
Michael


0001-Refactor-routine-to-test-connection-to-SSL-server.patch
Description: Binary data


0002-Support-channel-binding-tls-unique-in-SCRAM.patch
Description: Binary data


0003-Add-connection-parameters-saslname-and-saslchannelbi.patch
Description: Binary data


0004-Implement-channel-binding-tls-server-end-point-for-S.patch
Description: Binary data

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

Re: [HACKERS] Broken hint bits (freeze)

[Bruce Momjian]

On Mon, Jun 19, 2017 at 10:59:19PM -0400, Bruce Momjian wrote:
> On Tue, Jun 20, 2017 at 03:50:29AM +0300, Sergey Burladyan wrote:
> > 20 июн. 2017 г. 1:21 пользователь "Bruce Momjian"  
> > написал: 
> > 
> > 
> > We are saying that Log-Shipping should match "Latest checkpoint
> > location", but the WAL for that will not be sent to the standby, so it
> > will not match, but that is OK since the only thing in the non-shipped
> > WAL file is the checkpoint record.  How should we modify the wording on
> > this?
> > 
> > 
> > I am afraid that without this checkpoint record standby cannot make
> > restartpoint
> > and without restartpoint it does not sync shared buffers into disk at
> > shutdown. 
> 
> Uh, as I understand it the rsync is going to copy the missing WAL file
> from the new master to the standby, right, and I think pg_controldata
> too, so it should be fine.  Have you tested to see if it fails?

The point is that we are checking the "Latest checkpoint location" to
make sure all the WAL was replayed.   We are never going to start the
old standby server.  Rsync is going to copy the missing/changed files.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Getting server crash on Windows when using ICU collation

[Ashutosh Sharma]

Hi,

On Tue, Jun 20, 2017 at 2:36 AM, Peter Eisentraut
 wrote:
> On 6/19/17 00:42, Ashutosh Sharma wrote:
>>> If we don't find unconv, isn't it better to fall back to non-UTF8
>>> version rather than saying command not found?
>> Well, if any of the ICU package is installed on our system then we
>> will certainly find uconv command. The only case where we can see such
>> error is when user doesn't have any of the ICU packages installed on
>> their system and are somehow trying to perform icu enabled build and
>> in such case the build configuration has to fail which i think is the
>> expected behaviour. Anyways, it is not uconv that decides whether we
>> should fallback to non-UTF8 or not. It's the ICU version that decides
>> whether to stick to UTF8 or fallback to nonUTF8 version. Thanks.
>
> How do we know we're running the uconv command from the installation
> that we will compile against?

Okay, I think, you are talking about the case where we may have
multiple ICU versions installed on our system and there might be a
possibility that the uconv command may get executed from the ICU
version that we are not trying to link with postgres. This can only
happen when user has set the path for icu binaries in the system PATH
variable incorrectly. For e.g., if i have installed ICU versions 49
and 53 on my system and the library and include path i am trying to
add is from ICU version 53 whereas the system PATH points to dll's
from ICU 49 then that will be considered as a incorrect build setup.
In  this case, even if i am trying to use ICU 53 libraries but as
dll's i am using is from ICU 49 (as my system PATH is pointing to ICU
49 bin path) the uconv -V output would be '49.1' instead of '53.1'. In
such case, the postgres installation would fail because during
installation it would search for libicu53.dll not libicu49.dll.
Therefore, even if we are running wrong uconv command, we would not at
all succeed with the installation process. Hence, i think, we
shouldn't be worrying about any such things. Please let me know if i
could answer your question or not. Thanks.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


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

[HACKERS] Fix a typo in partition.c

[Masahiko Sawada]

Hi,

Attached patch for $subject.

s/opreator/operator/

Regards,

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


fix_typo_in_partition_c.patch
Description: Binary data

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

Re: [HACKERS] Broken hint bits (freeze)

[Bruce Momjian]

On Tue, Jun 20, 2017 at 03:50:29AM +0300, Sergey Burladyan wrote:
> 20 июн. 2017 г. 1:21 пользователь "Bruce Momjian"  написал: 
> 
> 
> We are saying that Log-Shipping should match "Latest checkpoint
> location", but the WAL for that will not be sent to the standby, so it
> will not match, but that is OK since the only thing in the non-shipped
> WAL file is the checkpoint record.  How should we modify the wording on
> this?
> 
> 
> I am afraid that without this checkpoint record standby cannot make
> restartpoint
> and without restartpoint it does not sync shared buffers into disk at
> shutdown. 

Uh, as I understand it the rsync is going to copy the missing WAL file
from the new master to the standby, right, and I think pg_controldata
too, so it should be fine.  Have you tested to see if it fails?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Get stuck when dropping a subscription during synchronizing table

[Masahiko Sawada]

On Tue, Jun 20, 2017 at 10:55 AM, Peter Eisentraut
 wrote:
> On 6/1/17 13:37, Petr Jelinek wrote:
>> On 01/06/17 17:32, Masahiko Sawada wrote:
>>> On Thu, May 25, 2017 at 5:29 PM, tushar  
>>> wrote:
 After applying all your patches, drop subscription no  more hangs while
 dropping  subscription but there is an error   "ERROR:  tuple concurrently
 updated" in the log file of
 standby.
>>>
>>> I tried to reproduce this issue with latest four patches I submit but
>>> it didn't happen. I guess this issue doesn't related to the issues
>>> reported on this thread and another factor  might be cause of this
>>> issue. It would be good to test the same again with latest four
>>> patches or after solved current some open items.
>>
>> That's because your additional patch kills the workers that do the
>> concurrent update. While that's probably okay, I still plan to look into
>> making the subscription and state locking more robust.
>
> It seems we have gotten off track here a bit.  What is the current
> proposal to fix "tuple concurrently updated" during DROP SUBSCRiPTION?

I think there is no proposal for it so far. The current proposal is to
fix this problem during ALTER SUBSCRIPTION.

> It seems to me we could just take a stronger lock around
> RemoveSubscriptionRel(), so that workers can't write in there concurrently.
>

Since we reduced the lock level of updating pg_subscription_rel by
commit 521fd4795e3e the same deadlock issue will appear if we just
take a stronger lock level.

Regards,

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


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

Re: [HACKERS] REPLICA IDENTITY FULL

[Tatsuo Ishii]

>> For example, if the table consists of only INT types, REPLICA IDENTITY
>> FULL works with UPDATE/DELETE (i.e. replicated), but if some of them
>> are TEXT types, then UPDATE/DELETE does not work.
>> 
>> See up thread for more details.
> 
> Right, but that's just a bug, nothing else.

If my understanding is correct, it would not be easy to fix, no?

> We might be able to refine that, but there is a general problem that
> without an index and an operator class, we are just doing our random
> best to match the values.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] REPLICA IDENTITY FULL

[Andres Freund]

On 2017-06-20 11:46:13 +0900, Tatsuo Ishii wrote:
> >> Yes, that's my understanding too. However, the feature may or may not
> >> work depending on the data types of columns, probably I will not
> >> recommend users/my customers to use it.
> > 
> > I'm not sure how datatypes are playing into this?
> 
> For example, if the table consists of only INT types, REPLICA IDENTITY
> FULL works with UPDATE/DELETE (i.e. replicated), but if some of them
> are TEXT types, then UPDATE/DELETE does not work.
> 
> See up thread for more details.

Right, but that's just a bug, nothing else.


-- 
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] REPLICA IDENTITY FULL

[Tatsuo Ishii]

>> Yes, that's my understanding too. However, the feature may or may not
>> work depending on the data types of columns, probably I will not
>> recommend users/my customers to use it.
> 
> I'm not sure how datatypes are playing into this?

For example, if the table consists of only INT types, REPLICA IDENTITY
FULL works with UPDATE/DELETE (i.e. replicated), but if some of them
are TEXT types, then UPDATE/DELETE does not work.

See up thread for more details.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] Get stuck when dropping a subscription during synchronizing table

[Masahiko Sawada]

On Tue, Jun 20, 2017 at 10:47 AM, Peter Eisentraut
 wrote:
> On 6/16/17 04:16, Masahiko Sawada wrote:
>> A subscription relation state may have been removed already when we
>> try to update it. SetSubscriptionRelState didn't emit an error in such
>> case but with this patch we end up with an error. Since we shouldn't
>> ignore such error in UpdateSubscriptionRelState I'd say we can let the
>> user know about that possibility in the error message.
>
> So are you saying it's good to have the error message?
>

Yes. UpdateSubscriptionRelState failure means that the subscription
relation state has disappeared or also means something wrong. So I
think it's good to have it as perhaps errdetail. Thought?

Regards,

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


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

Re: [HACKERS] REPLICA IDENTITY FULL

[Andres Freund]

On 2017-06-20 11:29:06 +0900, Tatsuo Ishii wrote:
> > Nothing, and that's what I do. I just think it's a bit fuzzy. Maybe
> > I'm misunderstanding the purpose of REPLICA IDENTITY, but I read it as
> > saying "this is the replication key for this relation".
> 
> Yes, that's my understanding too. However, the feature may or may not
> work depending on the data types of columns, probably I will not
> recommend users/my customers to use it.

I'm not sure how datatypes are playing into this?


-- 
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] REPLICA IDENTITY FULL

[Tatsuo Ishii]

> Nothing, and that's what I do. I just think it's a bit fuzzy. Maybe
> I'm misunderstanding the purpose of REPLICA IDENTITY, but I read it as
> saying "this is the replication key for this relation".

Yes, that's my understanding too. However, the feature may or may not
work depending on the data types of columns, probably I will not
recommend users/my customers to use it.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] PATCH: Batch/pipelining support for libpq

[Michael Paquier]

On Tue, Jun 20, 2017 at 10:43 AM, Craig Ringer  wrote:
> Especially with a 6-week-old baby now

Congratulations!
-- 
Michael


-- 
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] Rules on table partitions

[Amit Langote]

Hi Dean,

On 2017/06/19 20:19, Dean Rasheed wrote:
> Currently we allow rules to be defined on table partitions, but these
> rules only fire when the partition is accessed directly, not when it
> is accessed via the parent:

Yeah, the same thing as will happen with an inheritance setup, but I guess
you are asking whether that's what we want.

> CREATE TABLE t1(a int, b int) PARTITION BY RANGE(a);
> CREATE TABLE t1_p PARTITION OF t1 FOR VALUES FROM (1) TO (10);
> INSERT INTO t1 VALUES (1,101), (2,201);
> 
> CREATE TABLE t1_p_log(a int, b int, d date);
> CREATE RULE t1_p_log_rule AS ON UPDATE TO t1_p
>   DO ALSO INSERT INTO t1_p_log VALUES(old.a, old.b, now());
> 
> UPDATE t1 SET b=b+1 WHERE a=1;
> UPDATE t1_p SET b=b+1 WHERE a=2;
> 
> SELECT * FROM t1_p_log;
> 
>  a |  b  | d
> ---+-+
>  2 | 201 | 2017-06-19
> (1 row)
> 
> 
> I'd regard that as a bug, especially since this kind of thing would
> have worked with old-style user-defined partitioning.

With old-style inheritance based setup, you will see exactly the exact
same result:

create table p (a int, b int);
create table c () inherits (p);
create table c_log (a int, b int, c date);
create rule c_p_log_rule as on update to c
  do also insert into c_log values (old.a, old.b, now());
insert into p values (1, 100);
insert into c values (2, 200)

update p set b = b + 1 where a = 1;
select tableoid::regclass, * from p;
 tableoid | a |  b
--+---+-
 p| 1 | 101
 c| 2 | 200
(2 rows)

select * from c_log;
 a | b | c
---+---+---
(0 rows)

update p set b = b + 1 where a = 2;
select tableoid::regclass, * from p;
 tableoid | a |  b
--+---+-
 p| 1 | 101
 c| 2 | 201
(2 rows)

select * from c_log;
 a | b | c
---+---+---
(0 rows)

update c set b = b + 1 where a = 2;
select tableoid::regclass, * from p;
 tableoid | a |  b
--+---+-
 p| 1 | 101
 c| 2 | 202
(2 rows)

select * from c_log;
 a |  b  | c
---+-+
 2 | 201 | 2017-06-20
(1 row)

Note that inheritance is expanded in the planner, not the rewriter, so the
rules of partitions (that are added to the query later than the rewriter)
won't fire, AFAICS.  Same will apply to partitions.

> Perhaps we
> should explicitly forbid this for now -- i.e., raise a "not supported"
> error when attempting to add a rule to a partition, or attach a table
> with rules to a partitioned table.

We could do that, but an oft-raised question is how different we should
make new partitions from the old-style inheritance child tables?

Although a slightly different territory, you will also notice that
statement triggers of partitions won't be fired unless they are explicitly
named in the query, which is what happens for inheritance in general and
hence also for partitions.

> Personally, I wouldn't regard adding proper support for rules on
> partitions as a high priority, so I'd be OK with it remaining
> unsupported unless someone cares enough to implement it, but that
> seems preferable to leaving it partially working in this way.

Sure, if consensus turns out to be that we prohibit rules, statement
triggers, etc. that depend on the relation being explicitly named in the
query to be defined on partitions, I could draft up a patch for v10.

> Also, as things stand, it is possible to do the following:
> 
> CREATE TABLE t2(a int, b int) PARTITION BY RANGE(a);
> CREATE TABLE t2_p PARTITION OF t2 FOR VALUES FROM (1) TO (10);
> CREATE RULE "_RETURN" AS ON SELECT TO t2_p DO INSTEAD SELECT * FROM t2;
> 
> which results in the partition becoming a view that selects from the
> parent, which surely ought to be forbidden.

Hmm, yes.  The following exercise convinced me.

create table r (a int) partition by range (a);
create table r1 partition of r for values from (1) to (10);
create rule "_RETURN" as on select to r1 do instead select * from r;

insert into r values (1);
ERROR:  cannot insert into view "r1"
HINT:  To enable inserting into the view, provide an INSTEAD OF INSERT
trigger or an unconditional ON INSERT DO INSTEAD rule.

The error is emitted by CheckValidResultRel() that is called on individual
leaf partitions when setting up tuple-routing in ExecInitModifyTable.

I agree that we should forbid this case, so please find attached a patch.

Thanks,
Amit
diff --git a/src/backend/rewrite/rewriteDefine.c 
b/src/backend/rewrite/rewriteDefine.c
index fd3768de17..4213bafa27 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -428,6 +428,12 @@ DefineQueryRewrite(char *rulename,
errmsg("could not convert partitioned table 
\"%s\" to a view",
   
RelationGetRelationName(event_relation;
 
+   if (event_relation->rd_rel->relispartition)
+   ereport(ERROR,
+   
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+   

Re: [HACKERS] Something is rotten in publication drop

[Tom Lane]

Peter Eisentraut  writes:
> If there are no new insights, I plan to proceed with the attached patch
> tomorrow.  This leaves the existing view and function alone, adds
> pg_relation_is_publishable() and uses that in psql.

Hm, patch looks okay, but while eyeballing it I started to wonder
why in the world is pg_get_publication_tables marked prosecdef?
If that has any consequences at all, they're probably bad.
There are exactly no other built-in functions that have that set.

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] Get stuck when dropping a subscription during synchronizing table

[Peter Eisentraut]

On 6/1/17 13:37, Petr Jelinek wrote:
> On 01/06/17 17:32, Masahiko Sawada wrote:
>> On Thu, May 25, 2017 at 5:29 PM, tushar  
>> wrote:
>>> After applying all your patches, drop subscription no  more hangs while
>>> dropping  subscription but there is an error   "ERROR:  tuple concurrently
>>> updated" in the log file of
>>> standby.
>>
>> I tried to reproduce this issue with latest four patches I submit but
>> it didn't happen. I guess this issue doesn't related to the issues
>> reported on this thread and another factor  might be cause of this
>> issue. It would be good to test the same again with latest four
>> patches or after solved current some open items.
> 
> That's because your additional patch kills the workers that do the
> concurrent update. While that's probably okay, I still plan to look into
> making the subscription and state locking more robust.

It seems we have gotten off track here a bit.  What is the current
proposal to fix "tuple concurrently updated" during DROP SUBSCRiPTION?

It seems to me we could just take a stronger lock around
RemoveSubscriptionRel(), so that workers can't write in there concurrently.

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


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

Re: [HACKERS] REPLICA IDENTITY FULL

[Craig Ringer]

On 20 June 2017 at 09:47, Andres Freund  wrote:
> On 2017-06-20 09:45:27 +0800, Craig Ringer wrote:
>> I frequently want to be able to use REPLICA IDENTITY DEFAULT, but also
>> record the whole old tuple not just keys, so they can be used in
>> conflict processing etc.
>
> What stops you from automatically using a candidate key if available?

Nothing, and that's what I do. I just think it's a bit fuzzy. Maybe
I'm misunderstanding the purpose of REPLICA IDENTITY, but I read it as
saying "this is the replication key for this relation". If you use
REPLICA IDENTITY FULL then the replication tool goes "nah, I think
I'll pick the PK instead" is that really right?

It's not a major issue.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] REPLICA IDENTITY FULL

[Craig Ringer]

On 20 June 2017 at 06:53, Peter Eisentraut
 wrote:
> On 6/18/17 23:11, Tatsuo Ishii wrote:
>> While playing around with logical replication, I am confused by the
>> behavior of REPLICA IDENTITY FULL.
>
>> However, if a table has text columns, UPDATE/DELETE replication does
>> not work any more. Am I missing something?
>
> This is apparently because for replica identity full the comparison of
> the search key against the tuple value goes through datumIsEqual(),
> which doesn't work for TOAST values.

Personally I think REPLICA IDENTITY FULL conflates two related things.

One is "record the whole old value of the tuple in xlog so logical
decoding can access it".

Quite separately, there is "treat the full tuple as the replica identity key".

I frequently want to be able to use REPLICA IDENTITY DEFAULT, but also
record the whole old tuple not just keys, so they can be used in
conflict processing etc.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] Get stuck when dropping a subscription during synchronizing table

[Peter Eisentraut]

On 6/16/17 04:16, Masahiko Sawada wrote:
> A subscription relation state may have been removed already when we
> try to update it. SetSubscriptionRelState didn't emit an error in such
> case but with this patch we end up with an error. Since we shouldn't
> ignore such error in UpdateSubscriptionRelState I'd say we can let the
> user know about that possibility in the error message.

So are you saying it's good to have the error message?

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


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

Re: [HACKERS] REPLICA IDENTITY FULL

[Andres Freund]

On 2017-06-20 09:45:27 +0800, Craig Ringer wrote:
> I frequently want to be able to use REPLICA IDENTITY DEFAULT, but also
> record the whole old tuple not just keys, so they can be used in
> conflict processing etc.

What stops you from automatically using a candidate key if available?

- 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] PATCH: Batch/pipelining support for libpq

[Craig Ringer]

On 20 June 2017 at 06:49, Andres Freund  wrote:
> On 2017-04-05 15:45:26 -0700, Andres Freund wrote:
>> Hi,
>>
>> On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
>> > Regarding test patch, I have corrected the test suite after David Steele's
>> > comments.
>> > Also, I would like to mention that a companion patch was submitted by David
>> > Steele up-thread.
>> >
>> > Attached the latest code and test patch.
>>
>> My impression is that this'll need a couple more rounds of review. Given
>> that this'll establish API we'll pretty much ever going to be able to
>> change/remove, I think it'd be a bad idea to rush this into v10.
>> Therefore I propose moving this to the next CF.
>
> Craig, Vaishnavi, everyone else: Are you planning to continue to work on
> this for v11?  I'm willing to do another round, but only if it's
> worthwhile.

I'm happy to work on review, and will try to make some time, but have
to focus primarily on logical rep infrastructure. This patch was a
proof of concept and fun hack for me and while I'm glad folks are
interested, it's not something I can dedicate much time to. Especially
with a 6-week-old baby now

> FWIW, I still think this needs a pgbench or similar example integration,
> so we can actually properly measure the benefits.

I agree. I originally wanted to patch psql, but it's pretty intrusive.
pgbench is likely a better target. Also pg_restore.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] Something is rotten in publication drop

[Peter Eisentraut]

If there are no new insights, I plan to proceed with the attached patch
tomorrow.  This leaves the existing view and function alone, adds
pg_relation_is_publishable() and uses that in psql.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From e3cf8bc1d2ac44081c63bb5425ddde4f85ebeaf8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 19 Jun 2017 20:49:57 -0400
Subject: [PATCH v2] Tweak publication fetching in psql

Viewing a table with \d in psql also shows the publications at table is
in.  If a publication is concurrently dropped, this shows an error,
because the view pg_publication_tables internally uses
pg_get_publication_tables, which uses a catalog snapshot.  This can be
particularly annoying if a for-all-tables publication is concurrently
dropped.

To avoid that, write the query in psql differently.  Expose the function
pg_relation_is_publishable() to SQL and write the query using that.
That still has a risk of being affected by concurrent catalog changes,
but in this case it would be a table drop that causes problems, and then
the psql \d command wouldn't be interesting anymore anyway.

Reported-by: Tom Lane 
---
 src/backend/catalog/pg_publication.c | 24 
 src/bin/psql/describe.c  | 14 +-
 src/include/catalog/pg_proc.h|  2 ++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c 
b/src/backend/catalog/pg_publication.c
index 17105f4f2c..17b2e8d649 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -105,6 +105,30 @@ is_publishable_class(Oid relid, Form_pg_class reltuple)
relid >= FirstNormalObjectId;
 }
 
+
+/*
+ * SQL-callable variant of the above
+ *
+ * This returns null when the relation does not exist.  This is intended to be
+ * used for example in psql to avoid gratuitous errors when there are
+ * concurrent catalog changes.
+ */
+Datum
+pg_relation_is_publishable(PG_FUNCTION_ARGS)
+{
+   Oid relid = PG_GETARG_OID(0);
+   HeapTuple   tuple;
+   boolresult;
+
+   tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+   if (!tuple)
+   PG_RETURN_NULL();
+   result = is_publishable_class(relid, (Form_pg_class) GETSTRUCT(tuple));
+   ReleaseSysCache(tuple);
+   PG_RETURN_BOOL(result);
+}
+
+
 /*
  * Insert new publication / relation mapping.
  */
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ea0e8af2ec..0e19e94841 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2536,12 +2536,16 @@ describeOneTableDetails(const char *schemaname,
if (pset.sversion >= 10)
{
printfPQExpBuffer(,
- "SELECT pub.pubname\n"
- " FROM 
pg_catalog.pg_publication pub,\n"
- "  
pg_catalog.pg_get_publication_tables(pub.pubname)\n"
- "WHERE relid = '%s'\n"
+ "SELECT pubname\n"
+ "FROM 
pg_catalog.pg_publication p\n"
+ "JOIN 
pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
+ "WHERE pr.prrelid = 
'%s'\n"
+ "UNION ALL\n"
+ "SELECT pubname\n"
+ "FROM 
pg_catalog.pg_publication p\n"
+ "WHERE p.puballtables 
AND pg_relation_is_publishable('%s')\n"
  "ORDER BY 1;",
- oid);
+ oid, oid);
 
result = PSQLexec(buf.data);
if (!result)
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6c44def6e6..81bed23426 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5436,6 +5436,8 @@ DESCR("get progress for all replication origins");
 /* publications */
 DATA(insert OID = 6119 ( pg_get_publication_tables PGNSP PGUID 12 1 1000 0 
0 f f t f t t s s 1 0 26 "25" "{25,26}" "{i,o}" "{pubname,relid}" _null_ _null_ 
pg_get_publication_tables _null_ _null_ _null_ ));
 DESCR("get OIDs of tables in a publication");
+DATA(insert OID = 6121 ( pg_relation_is_publishablePGNSP PGUID 12 10 0 
0 f f f f t f s s 1 0 16 "2205" _null_ _null_ _null_ _null_ 

Re: [HACKERS] Broken hint bits (freeze)

[Sergey Burladyan]

20 июн. 2017 г. 1:21 пользователь "Bruce Momjian" 
написал:


We are saying that Log-Shipping should match "Latest checkpoint
location", but the WAL for that will not be sent to the standby, so it
will not match, but that is OK since the only thing in the non-shipped
WAL file is the checkpoint record.  How should we modify the wording on
this?


I am afraid that without this checkpoint record standby cannot make
restartpoint
and without restartpoint it does not sync shared buffers into disk at
shutdown.

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Vaishnavi Prabakaran]

On Tue, Jun 20, 2017 at 8:49 AM, Andres Freund  wrote:

> On 2017-04-05 15:45:26 -0700, Andres Freund wrote:
> > Hi,
> >
> > On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
> > > Regarding test patch, I have corrected the test suite after David
> Steele's
> > > comments.
> > > Also, I would like to mention that a companion patch was submitted by
> David
> > > Steele up-thread.
> > >
> > > Attached the latest code and test patch.
> >
> > My impression is that this'll need a couple more rounds of review. Given
> > that this'll establish API we'll pretty much ever going to be able to
> > change/remove, I think it'd be a bad idea to rush this into v10.
> > Therefore I propose moving this to the next CF.
>
> Craig, Vaishnavi, everyone else: Are you planning to continue to work on
> this for v11?  I'm willing to do another round, but only if it's
> worthwhile.
>

Yes, am willing to continue working on this patch for v11.


>
> FWIW, I still think this needs a pgbench or similar example integration,
> so we can actually properly measure the benefits.
>

I will investigate on this further.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

Re: [HACKERS] REPLICA IDENTITY FULL

[Tatsuo Ishii]

> This is apparently because for replica identity full the comparison of
> the search key against the tuple value goes through datumIsEqual(),
> which doesn't work for TOAST values.
> 
> We might be able to refine that, but there is a general problem that
> without an index and an operator class, we are just doing our random
> best to match the values.

In other word, pass-by-value types work in this case? If so, we can
document it or throw an error while executing ALTER REPLICA IDENTITY
FULL on tables consisting of non pass-by-values column types to
mitigate the problem.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] REPLICA IDENTITY FULL

[Peter Eisentraut]

On 6/18/17 23:11, Tatsuo Ishii wrote:
> While playing around with logical replication, I am confused by the
> behavior of REPLICA IDENTITY FULL.

> However, if a table has text columns, UPDATE/DELETE replication does
> not work any more. Am I missing something?

This is apparently because for replica identity full the comparison of
the search key against the tuple value goes through datumIsEqual(),
which doesn't work for TOAST values.

We might be able to refine that, but there is a general problem that
without an index and an operator class, we are just doing our random
best to match the values.

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


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

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Andres Freund]

On 2017-04-05 15:45:26 -0700, Andres Freund wrote:
> Hi,
> 
> On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
> > Regarding test patch, I have corrected the test suite after David Steele's
> > comments.
> > Also, I would like to mention that a companion patch was submitted by David
> > Steele up-thread.
> > 
> > Attached the latest code and test patch.
> 
> My impression is that this'll need a couple more rounds of review. Given
> that this'll establish API we'll pretty much ever going to be able to
> change/remove, I think it'd be a bad idea to rush this into v10.
> Therefore I propose moving this to the next CF.

Craig, Vaishnavi, everyone else: Are you planning to continue to work on
this for v11?  I'm willing to do another round, but only if it's
worthwhile.

FWIW, I still think this needs a pgbench or similar example integration,
so we can actually properly measure the benefits.

- 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] Broken hint bits (freeze)

[Bruce Momjian]

On Sat, Jun 17, 2017 at 08:34:47AM +0530, Amit Kapila wrote:
> On Fri, Jun 16, 2017 at 11:03 PM, Sergey Burladyan  
> wrote:
> >> > Yeah, we have ensured that all the transactions before shutdown
> >> > checkpoint got archived.  It is done in commit
> >> > 2e6107cb621d003dcab0df53ac8673ea67c4e467.  However, it is not clear to
> >> > me neither it is mentioned in comments why we have done it that way.
> >>
> >> Yes, I am confused why Sergey doesn't see that behavior.
> >
> 
> The behavior reported by Sergey is what is expected i.e the last file
> in which shutdown checkpoint record is written won't be archived and
> there is a reason behind that.  We always perform shutdown checkpoint
> (which will write shutdown checkpoint record) after requesting a xlog
> switch.  Any record written after xlog switch won't be archived unless
> it is so big that it consumes complete xlog segment.
> 
> > I think this last new switched WAL with shutdown checkpoint record is
> > incomplete and it does not marked as *.ready in pg_xlog/archive_status/
> > and not archived.
> >
> 
> Yes, that's true and is expected behavior.

OK, so our pg_upgrade documentation is currently incorrect:

https://www.postgresql.org/docs/10/static/pgupgrade.html

8. Verify standby servers

If you are upgrading Streaming Replication and Log-Shipping standby
servers, verify that the old standby servers are caught up by running
pg_controldata against the old primary and standby clusters. Verify that
the "Latest checkpoint location" values match in all clusters. (There
will be a mismatch if old standby servers were shut down before the old
primary.)

We are saying that Log-Shipping should match "Latest checkpoint
location", but the WAL for that will not be sent to the standby, so it
will not match, but that is OK since the only thing in the non-shipped
WAL file is the checkpoint record.  How should we modify the wording on
this?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Mark Rofail wrote:
> Okay, so major breakthrough.
> 
> *Updates:*
> 
>- The operator @>(anyarray, anyelement) is now functional
>   - The segmentation fault was due to applying PG_FREE_IF_COPY on a
>   datum when it should only be applied on TOASTed inputs
>   - The only problem now is if for example you apply the operator as
>   follows  '{AA646'}' @> 'AA646' it maps to @>(anyarray,
>   anyarray) since 'AA646' is interpreted as char[] instead of Text
>- Added some regression tests (src/test/regress/sql/arrays.sql) and
>their results(src/test/regress/expected/arrays.out)
>- wokred on the new GIN strategy, I don't think it would vary much from
>GinContainsStrategy.

OK, that's great.

> *What I plan to do:*
> 
>- I need to start working on the Referential Integrity code but I don't
>where to start

You need to study the old patch posted by Marco Nenciarini.

-- 
Á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] transition table behavior with inheritance appears broken

[Robert Haas]

On Sun, Jun 18, 2017 at 6:59 PM, Andrew Gierth
 wrote:
> (Any preferences for whether it should be one commit or 3 separate ones?)

If I were doing it, I would commit them separately.

But I'm not doing it, so I won't complain about what you decide to do.

-- 
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] UPDATE of partition key

[Robert Haas]

On Thu, Jun 15, 2017 at 1:36 PM, Amit Khandekar  wrote:
> Attached patch v10 fixes the above. In the existing code, where it
> builds WCO constraints for each leaf partition; with the patch, that
> code now is applicable to row-movement-updates as well.

I guess I don't see why it should work like this.  In the INSERT case,
we must build withCheckOption objects for each partition because those
partitions don't appear in the plan otherwise -- but in the UPDATE
case, they're already there, so why do we need to build anything at
all?  Similarly for RETURNING projections.  How are the things we need
for those cases not already getting built, associated with the
relevant resultRelInfos?  Maybe there's a concern if some children got
pruned - they could turn out later to be the children into which
tuples need to be routed.  But the patch makes no distinction between
possibly-pruned children and any others.

> There is another issue I discovered. The row-movement works fine if
> the destination leaf partition has different attribute ordering than
> the root : the existing insert-tuple-routing mapping handles that. But
> if the source partition has different ordering w.r.t. the root, it has
> a problem : there is no mapping in the opposite direction, i.e. from
> the leaf to root. And we require that because the tuple of source leaf
> partition needs to be converted to root partition tuple descriptor,
> since ExecFindPartition() starts with root.

Seems reasonable, but...

> To fix this, I have introduced another mapping array
> mtstate->mt_resultrel_maps[]. This corresponds to the
> mtstate->resultRelInfo[]. We don't require per-leaf-partition mapping,
> because the update result relations are pruned subset of the total
> leaf partitions.

... I don't understand how you can *not* need a per-leaf-partition
mapping.  I mean, maybe you only need the mapping for the *unpruned*
leaf partitions but you certainly need a separate mapping for each one
of those.

It's possible to imagine driving the tuple routing off of just the
partition key attributes, extracted from wherever they are inside the
tuple at the current level, rather than converting to the root's tuple
format.  However, that's not totally straightforward because there
could be multiple levels of partitioning throughout the tree and
different attributes might be needed at different levels.  Moreover,
in most cases, the mappings are going to end up being no-ops because
the column order will be the same, so it's probably not worth
complicating the code to try to avoid a double conversion that usually
won't happen.

-- 
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] psql's \d and \dt are sending their complaints to different output files

[Tom Lane]

"David G. Johnston"  writes:
> On Mon, Jun 19, 2017 at 2:53 PM, Tom Lane  wrote:
>> Where are you reading that?

> https://www.postgresql.org/docs/9.6/static/app-psql.html

> First sentence:
> "For each relation (table, view, index, sequence, or foreign table) or
> composite type matching the pattern..."

Ah.  Clearly missed in the matviews patch (which I now see also missed
updating some relevant comments, though it did change the code).

Will fix.

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] UPDATE of partition key

[Thomas Munro]

On Fri, Jun 16, 2017 at 5:36 AM, Amit Khandekar  wrote:
> There is another issue I discovered. The row-movement works fine if
> the destination leaf partition has different attribute ordering than
> the root : the existing insert-tuple-routing mapping handles that. But
> if the source partition has different ordering w.r.t. the root, it has
> a problem : there is no mapping in the opposite direction, i.e. from
> the leaf to root. And we require that because the tuple of source leaf
> partition needs to be converted to root partition tuple descriptor,
> since ExecFindPartition() starts with root.
>
> To fix this, I have introduced another mapping array
> mtstate->mt_resultrel_maps[]. This corresponds to the
> mtstate->resultRelInfo[]. We don't require per-leaf-partition mapping,
> because the update result relations are pruned subset of the total
> leaf partitions.

Hi Amit & Amit,

Just a thought: If I understand correctly this new array of tuple
conversion maps is the same as mtstate->mt_transition_tupconv_maps in
my patch transition-tuples-from-child-tables-v11.patch (hopefully soon
to be committed to close a PG10 open item).  In my patch I bounce
transition tuples from child relations up to the named relation's
triggers, and in this patch you bounce child tuples up to the named
relation for rerouting, so the conversion requirement is the same.
Perhaps we could consider refactoring to build a common struct member
on demand for the row movement patch at some point in the future if it
makes the code cleaner.

-- 
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] psql's \d and \dt are sending their complaints to different output files

[David G. Johnston]

On Mon, Jun 19, 2017 at 2:53 PM, Tom Lane  wrote:
> "David G. Johnston"  writes:
>> The docs also indicate that we don't include materialized views as
>> part of "\d" which seems like an oversight somewhere.
>
> Where are you reading that?

https://www.postgresql.org/docs/9.6/static/app-psql.html

First sentence:

"For each relation (table, view, index, sequence, or foreign table) or
composite type matching the pattern..."

I was expecting "materialized view" to be one of the parenthetical options.

> Experimentation shows that "\d" does include
> matviews, and that matches the code, which has this as the default
> expansion of \d:
>
> /* standard listing of interesting things */
> success = listTables("tvmsE", NULL, show_verbose, 
> show_system);
>

\dT / "composite type" seems to be a special case.

David J.


-- 
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] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Okay, so major breakthrough.

*Updates:*

   - The operator @>(anyarray, anyelement) is now functional
  - The segmentation fault was due to applying PG_FREE_IF_COPY on a
  datum when it should only be applied on TOASTed inputs
  - The only problem now is if for example you apply the operator as
  follows  '{AA646'}' @> 'AA646' it maps to @>(anyarray,
  anyarray) since 'AA646' is interpreted as char[] instead of Text
   - Added some regression tests (src/test/regress/sql/arrays.sql) and
   their results(src/test/regress/expected/arrays.out)
   - wokred on the new GIN strategy, I don't think it would vary much from
   GinContainsStrategy.

*What I plan to do:*

   - I need to start working on the Referential Integrity code but I don't
   where to start

Best Regards,
Mark Rofail
diff --git a/src/backend/access/gin/ginarrayproc.c b/src/backend/access/gin/ginarrayproc.c
index cc7435e030..a1b3f53ed9 100644
--- a/src/backend/access/gin/ginarrayproc.c
+++ b/src/backend/access/gin/ginarrayproc.c
@@ -24,6 +24,7 @@
 #define GinContainsStrategy		2
 #define GinContainedStrategy	3
 #define GinEqualStrategy		4
+#define GinContainsElemStrategy	5
 
 
 /*
@@ -110,6 +111,11 @@ ginqueryarrayextract(PG_FUNCTION_ARGS)
 		case GinOverlapStrategy:
 			*searchMode = GIN_SEARCH_MODE_DEFAULT;
 			break;
+		case GinContainsElemStrategy:
+			/* only items that match the queried element 
+are considered candidate  */
+			*searchMode = GIN_SEARCH_MODE_DEFAULT;
+			break;
 		case GinContainsStrategy:
 			if (nelems > 0)
 *searchMode = GIN_SEARCH_MODE_DEFAULT;
@@ -171,6 +177,7 @@ ginarrayconsistent(PG_FUNCTION_ARGS)
 }
 			}
 			break;
+		case GinContainsElemStrategy:
 		case GinContainsStrategy:
 			/* result is not lossy */
 			*recheck = false;
@@ -258,7 +265,8 @@ ginarraytriconsistent(PG_FUNCTION_ARGS)
 }
 			}
 			break;
-		case GinContainsStrategy:
+			case GinContainsElemStrategy:
+			case GinContainsStrategy:
 			/* must have all elements in check[] true, and no nulls */
 			res = GIN_TRUE;
 			for (i = 0; i < nkeys; i++)
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index d9c8aa569c..c563aa564e 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -4232,6 +4232,117 @@ arraycontained(PG_FUNCTION_ARGS)
 	PG_RETURN_BOOL(result);
 }
 
+/*
+ * array_contains_elem : checks an array for a spefific element
+ */
+static bool
+array_contains_elem(AnyArrayType *array, Datum elem, Oid element_type,
+bool element_isnull, Oid collation,	void **fn_extra)
+{
+	Oid 		arr_type = AARR_ELEMTYPE(array);
+	TypeCacheEntry *typentry;
+	int 		nelems;
+	int			typlen;
+	bool		typbyval;
+	char		typalign;
+	int			i;
+	array_iter 	it1;
+	FunctionCallInfoData locfcinfo;
+
+	if (arr_type != element_type)
+		ereport(ERROR,
+(errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("cannot compare different element types")));
+	
+	if (element_isnull)
+		return false;
+		
+	/*
+	 * We arrange to look up the equality function only once per series of
+	 * calls, assuming the element type doesn't change underneath us.  The
+	 * typcache is used so that we have no memory leakage when being used as
+	 * an index support function.
+	 */
+	typentry = (TypeCacheEntry *)*fn_extra;
+	if (typentry == NULL ||
+		typentry->type_id != arr_type)
+	{
+		typentry = lookup_type_cache(arr_type,
+	 TYPECACHE_EQ_OPR_FINFO);
+		if (!OidIsValid(typentry->eq_opr_finfo.fn_oid))
+			ereport(ERROR,
+	(errcode(ERRCODE_UNDEFINED_FUNCTION),
+	 errmsg("could not identify an equality operator for type %s",
+			format_type_be(arr_type;
+		*fn_extra = (void *)typentry;
+	}
+	typlen = typentry->typlen;
+	typbyval = typentry->typbyval;
+	typalign = typentry->typalign;
+
+	/*
+	 * Apply the comparison operator to each pair of array elements.
+	 */
+	InitFunctionCallInfoData(locfcinfo, >eq_opr_finfo, 2,
+			 collation, NULL, NULL);
+
+	/* Loop over source data */
+	nelems = ArrayGetNItems(AARR_NDIM(array), AARR_DIMS(array));
+	array_iter_setup(, array);
+
+	for (i = 0; i < nelems; i++)
+	{
+		Datum elt1;
+		bool isnull;
+		bool oprresult;
+
+		/* Get element, checking for NULL */
+		elt1 = array_iter_next(, , i, typlen, typbyval, typalign);
+
+		/*
+		 * We assume that the comparison operator is strict, so a NULL can't
+		 * match anything.  XXX this diverges from the "NULL=NULL" behavior of
+		 * array_eq, should we act like that?
+		 */
+		if (isnull)
+			continue;
+
+		/*
+			* Apply the operator to the element pair
+			*/
+		locfcinfo.arg[0] = elt1;
+		locfcinfo.arg[1] = elem;
+		locfcinfo.argnull[0] = false;
+		locfcinfo.argnull[1] = false;
+		locfcinfo.isnull = false;
+		oprresult = DatumGetBool(FunctionCallInvoke());
+		if (oprresult)
+			return true;
+	}
+
+	return false;
+}
+
+Datum
+arraycontainselem(PG_FUNCTION_ARGS)
+{
+	AnyArrayType *array = PG_GETARG_ANY_ARRAY(0);
+	Datum elem = PG_GETARG_DATUM(1);
+	Oid element_type = 

Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files

[Tom Lane]

"David G. Johnston"  writes:
> The docs also indicate that we don't include materialized views as
> part of "\d" which seems like an oversight somewhere.

Where are you reading that?  Experimentation shows that "\d" does include
matviews, and that matches the code, which has this as the default
expansion of \d:

/* standard listing of interesting things */
success = listTables("tvmsE", NULL, show_verbose, show_system);

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] psql's \d and \dt are sending their complaints to different output files

[David G. Johnston]

On Mon, Jun 19, 2017 at 2:25 PM, Peter Eisentraut
 wrote:
> On 6/19/17 09:00, Oleksandr Shulgin wrote:
>> I wonder if it is intentional that \d complains on stderr if it cannot
>> find relations to match, but \dt prints the message to the current
>> output file?
>>
>> postgres=# \d xxx
>> Did not find any relation named "xxx".
>> postgres=# \dt xxx
>> No matching relations found.
>>>
> The first command is "show me relation xxx" and that gives an error
> message if it does not exist (and would also create an appropriate exit
> status if certain options are used).
>
> The second command says "show me all relations matched 'xxx'".  The
> result of this is successful execution showing nothing.  The message it
> prints is a "courtesy" message.

The docs indicate the optional argument to both is a pattern so I'm
not seeing why they are treated differently.

The docs also indicate that we don't include materialized views as
part of "\d" which seems like an oversight somewhere.

It sounds like, though isn't specified anywhere, the while you can
write "\dit" to get a subset of all available options omitting all of
the options leaves you with "\d" which is equivalent to specifying
them all.  Which leads on to believe the output from both should be in
sync.

David J.


-- 
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] psql's \d and \dt are sending their complaints to different output files

[Tom Lane]

Peter Eisentraut  writes:
> On 6/19/17 09:00, Oleksandr Shulgin wrote:
>> postgres=# \d xxx
>> Did not find any relation named "xxx".
>> postgres=# \dt xxx
>> No matching relations found.

> I think this is intentional.

> The first command is "show me relation xxx", and that gives an error
> message if it does not exist (and would also create an appropriate exit
> status if certain options are used).
> The second command says "show me all relations matched 'xxx'".  The
> result of this is successful execution showing nothing.  The message it
> prints is a "courtesy" message.

I don't buy that line of argument one bit, because both commands take
pattern arguments.

regression=# \d fool*
Did not find any relation named "fool*".
regression=# \dt fool*
No matching relations found.
regression=# create table fool1(f1 int);
CREATE TABLE
regression=# create table fool2(f1 int);
CREATE TABLE
regression=# \d fool*
   Table "public.fool1"
 Column |  Type   | Collation | Nullable | Default 
+-+---+--+-
 f1 | integer |   |  | 

   Table "public.fool2"
 Column |  Type   | Collation | Nullable | Default 
+-+---+--+-
 f1 | integer |   |  | 

regression=# \dt fool*
 List of relations
 Schema | Name  | Type  |  Owner   
+---+---+--
 public | fool1 | table | postgres
 public | fool2 | table | postgres
(2 rows)

AFAICS, this is just randomly different responses for identical
situations.

There's certainly room to quibble about whether this is an error
condition or not, but I'm not very sure which side of that argument
I'd come down on.  In any case the main point is that different
\d commands are doing different things for no very obvious reason.

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] CREATE SUBSCRIPTION documentation

[Peter Eisentraut]

On 6/19/17 13:28, Jeff Janes wrote:
> I think it would be better to instead reference:
> 
> https://www.postgresql.org/docs/devel/static/logical-replication-security.html
> 
> And on that page, it says:
> 
> "The role used for the replication connection must have
> the |REPLICATION| attribute"
> 
> Should also say something like "or be a superuser".  It is bit tedious
> to say that everywhere, but the docs are supposed to be a bit tedious.

Fixed, thanks!

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


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

Re: [BUGS] [HACKERS] Re: Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Chapman Flack]

On 06/19/2017 05:19 PM, David G. Johnston wrote:

> At first glance I think I'd rather have it do the correct thing all of
> the time, even if it takes longer, so that my only trade-off decision
> is whether to improve performance by fixing the application.
> 
> Ideally if the input tuple wouldn't require compression we wouldn't
> bother to decompress the stored tuple.

That looks like one reasonable elimination check.

I wonder how much closer it could get with some changes that wouldn't
necessarily use many more cycles.

One might be a less_easy queue; marching through the tuple
comparing fields, if one is found to be TOASTed, throw it
on the queue and march on. Only if all the easy ones matched
is there any point in looking at the queue.

At that point, there could be a tunable for how much effort
to expend. Perhaps I'm willing to decompress an inline value,
but not retrieve an out-of-line one? For the TOAST compression
algorithm I'm not sure of the balance between compression
and decompression effort; I know gzip decompression is pretty cheap.

-Chap


-- 
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] psql's \d and \dt are sending their complaints to different output files

[Peter Eisentraut]

On 6/19/17 09:00, Oleksandr Shulgin wrote:
> I wonder if it is intentional that \d complains on stderr if it cannot
> find relations to match, but \dt prints the message to the current
> output file?
> 
> postgres=# \d xxx
> Did not find any relation named "xxx".
> postgres=# \dt xxx
> No matching relations found.
> 
> I've noticed the difference exactly because my output was
> (accidentally) redirected to a file and I didn't see the complaint from
> the 2nd backslash command.

I think this is intentional.

The first command is "show me relation xxx", and that gives an error
message if it does not exist (and would also create an appropriate exit
status if certain options are used).

The second command says "show me all relations matched 'xxx'".  The
result of this is successful execution showing nothing.  The message it
prints is a "courtesy" message.

Maybe there is something to tweak here, but the basic distinction of
what is an error and what isn't should be preserved.

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


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

Re: [BUGS] [HACKERS] Re: Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[David G. Johnston]

On Mon, Jun 19, 2017 at 2:10 PM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Tom Lane wrote:
>>> ... If the trigger is succeeding (ie,
>>> detecting a no-op update) often enough that it would be worth that,
>>> you've really got an application-stupidity problem to fix.
>
>> ISTM the whole point of suppress_redundant_updates_trigger is to cope
>> with application stupidity.
>
> I think it's a suitable band-aid for limited amounts of stupidity.
> But it eliminates only a small fraction of the total overhead involved
> in a useless update command.  So I remain of the opinion that if that's
> happening a lot, you're better off fixing the problem somewhere upstream.

At first glance I think I'd rather have it do the correct thing all of
the time, even if it takes longer, so that my only trade-off decision
is whether to improve performance by fixing the application.

Ideally if the input tuple wouldn't require compression we wouldn't
bother to decompress the stored tuple.

David J.


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

Re: [BUGS] [HACKERS] Re: Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> ... If the trigger is succeeding (ie,
>> detecting a no-op update) often enough that it would be worth that,
>> you've really got an application-stupidity problem to fix.

> ISTM the whole point of suppress_redundant_updates_trigger is to cope
> with application stupidity.

I think it's a suitable band-aid for limited amounts of stupidity.
But it eliminates only a small fraction of the total overhead involved
in a useless update command.  So I remain of the opinion that if that's
happening a lot, you're better off fixing the problem somewhere upstream.

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] Rules on table partitions

[Peter Eisentraut]

On 6/19/17 07:19, Dean Rasheed wrote:
> Currently we allow rules to be defined on table partitions, but these
> rules only fire when the partition is accessed directly, not when it
> is accessed via the parent

That's what I would have expected, but I realize that there are always
multiple ways to interpret things in this area.

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


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

Re: [HACKERS] Getting server crash on Windows when using ICU collation

[Peter Eisentraut]

On 6/19/17 00:42, Ashutosh Sharma wrote:
>> If we don't find unconv, isn't it better to fall back to non-UTF8
>> version rather than saying command not found?
> Well, if any of the ICU package is installed on our system then we
> will certainly find uconv command. The only case where we can see such
> error is when user doesn't have any of the ICU packages installed on
> their system and are somehow trying to perform icu enabled build and
> in such case the build configuration has to fail which i think is the
> expected behaviour. Anyways, it is not uconv that decides whether we
> should fallback to non-UTF8 or not. It's the ICU version that decides
> whether to stick to UTF8 or fallback to nonUTF8 version. Thanks.

How do we know we're running the uconv command from the installation
that we will compile against?

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


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

Re: [BUGS] [HACKERS] Re: Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Alvaro Herrera]

Tom Lane wrote:

> As I mentioned upthread, it'd certainly be possible for the trigger
> to iterate through the fields in a datatype-aware fashion and undo
> compression or out-of-lineing before the comparison.  But that would
> eat a lot more cycles than the current implementation, and it seems
> dubious that it's worth it.  If the trigger is succeeding (ie,
> detecting a no-op update) often enough that it would be worth that,
> you've really got an application-stupidity problem to fix.

ISTM the whole point of suppress_redundant_updates_trigger is to cope
with application stupidity.

-- 
Á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] Re: Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Tom Lane]

J Chapman Flack  writes:
> On 06/19/2017 11:40 AM, Dilip Kumar wrote:
>> ... Artus de benque ... wrote:
>>> ...=# UPDATE test_table SET field = rpad('', 2001, 'a') WHERE id = 1;

>> Seems like in "suppress_redundant_updates_trigger"  we are comparing
>> toasted tuple with the new tuple and that is the cause of the bug.

> Something still puzzles me about this, though, maybe only because
> I don't know enough about TOAST.

> The size of 'field' ends up 2001, or just over the threshold where
> TOASTing will be attempted at all. The report doesn't mention changing
> the strategy from the default EXTENDED, so won't the first thing
> attempted be compression? Won't that succeed spectacularly, since the
> test string is a single character 2001 times, probably producing
> a compressed string a handful of bytes long, well under the threshold,
> obviating any need to go further with TOAST pointers?

Right, given the facts at hand, the stored old tuple has probably
got a compressed-in-line version of "field".  However, the *new*
tuple is a transient tuple containing the uncompressed result of
rpad().  We don't bother to try to compress fields or shove them
out-of-line until after all the BEFORE ROW triggers are done ---
if we did, the effort might just be wasted, if the triggers change
those fields or cancel the update altogether.  So the trigger is
seeing a compressed vs. an uncompressed version of the field value,
and since it's just doing a dumb bitwise compare, they don't look
equal.

As I mentioned upthread, it'd certainly be possible for the trigger
to iterate through the fields in a datatype-aware fashion and undo
compression or out-of-lineing before the comparison.  But that would
eat a lot more cycles than the current implementation, and it seems
dubious that it's worth it.  If the trigger is succeeding (ie,
detecting a no-op update) often enough that it would be worth that,
you've really got an application-stupidity problem to fix.

> Is the compression algorithm nondeterministic?

I don't think so.

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] Regression in join selectivity estimations when using foreign keys

[Tom Lane]

David Rowley  writes:
> On 22 May 2017 at 16:10, David Rowley  wrote:
>> I also just noticed that I don't think I've got ANTI join cases
>> correct in the patch I sent. I'll look at that now.

> I've attached an updated patch.
> This one is much less invasive than my original attempt.

Sorry for not dealing with this sooner --- I put it on the back burner
for PGCon, and just now got back to it.

First off, I agree that it was probably a mistake for this code to
special-case LEFT/FULL joins at all.  eqjoinsel() does not do so;
it relies on the join-type correction applied later by
calc_joinrel_size_estimate().  Since that correction is also downstream
of this code, we should be able to do the same here, and indeed may
be double-counting somehow if we don't.

What that leaves is that we're using the "smallest per-column selectivity"
hack only for SEMI/ANTI joins where we can't really get anything helpful
from knowledge of the FK.  What your patch does is to fall back on the
traditional clauselist_selectivity calculation for the relevant clauses.
But we'd be better off ignoring the FK altogether and leaving those
clauses to be processed later.  That saves some cycles, and it might allow
those clauses to be used more fruitfully with a different FK, and even
if that doesn't happen it's better to let clauselist_selectivity see as
many clauses at once as possible.

So I whacked the patch around to do it like that and pushed it.

I'm not totally satisfied that there isn't any case where the smallest
selectivity hack is appropriate.  In the example you're showing here,
the FK columns are independent so that we get more or less the right
answer with or without the FK.  But in some quick testing I could not
produce a counterexample proving that that heuristic is helpful;
so for now let's can it.

Thanks, and sorry again for the delay.

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] Decimal64 and Decimal128

[Peter Geoghegan]

On Mon, Jun 19, 2017 at 12:19 PM, Robert Haas  wrote:
> I don't have a specific use case in mind.  However, datumCopy() is
> sure to be a lot faster when typByVal is true, and see also the
> documentation changes in commit
> 8472bf7a73487b0535c95e299773b882f7523463.

Fair enough.

I ask because at one time I informally benchmarked Postgres (using
pgbench), where int4 (or maybe int8) primary keys were replaced with
equivalent numeric primary keys. This was a SELECT benchmark. Anyway,
the conclusion at the time was that it makes surprisingly little
difference (I think it was ~5%), because cache misses dominate anyway,
and the page layout doesn't really change (the fan-in didn't change
*at all* either, at least for this one case, because of alignment
considerations). I never published this result, because I didn't have
time to test rigorously, and wasn't sure that there was sufficient
interest.

This was intended to confirm my intuition that cache misses were by
far the main bottleneck (profiling also helped). I was thinking about
putting abbreviated keys within internal B-Tree pages at the time
(probably interleaved with the ItemIdData array). I've since realized
that prefix compression is more or less prerequisite (to get value
from a 1 or 2 byte abbreviated key), and that there are some painful
issues with collations + text. You probably need to encode each
internal page IndexTuple as a simple binary string that you always
just memcmp() in a type/tuple descriptor agnostic fashion, leaving
compression, truncation, and abbreviation as relatively trivial tasks.
This is all very difficult, of course, which is why it wasn't
seriously pursued.

-- 
Peter Geoghegan


-- 
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] Preliminary results for proposed new pgindent implementation

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut
> >>  wrote:
> >>> On 6/16/17 10:51, Tom Lane wrote:
>  So I'm back to the position that we ought to stick the indent
>  code under src/tools/ in our main repo.  Is anyone really
>  seriously against that?
> 
> >>> I think it would be better to have it separate.
> 
> >> +1.
> 
> > +1.
> 
> Given the license issues raised downthread, we have no choice in
> the short term.  So I have a request in to create a separate repo
> on git.postgresql.org (whose chain do I need to pull to get that
> approved, btw?)

u, that would probably be pginfra in some capacity, but I don't
recall seeing any notification of such a request.

I will follow up with those responsible,  #blamemagnus

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Preliminary results for proposed new pgindent implementation

[Tom Lane]

Stephen Frost  writes:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut
>>  wrote:
>>> On 6/16/17 10:51, Tom Lane wrote:
 So I'm back to the position that we ought to stick the indent
 code under src/tools/ in our main repo.  Is anyone really
 seriously against that?

>>> I think it would be better to have it separate.

>> +1.

> +1.

Given the license issues raised downthread, we have no choice in
the short term.  So I have a request in to create a separate repo
on git.postgresql.org (whose chain do I need to pull to get that
approved, btw?)

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] Decimal64 and Decimal128

[Robert Haas]

On Mon, Jun 19, 2017 at 1:10 PM, Peter Geoghegan  wrote:
> On Mon, Jun 19, 2017 at 10:00 AM, Robert Haas  wrote:
>> I've never been very happy with the performance of numeric, so I guess
>> I'm a bit more optimistic about the chances of doing better.  Aside
>> from any computational optimizations, the fact that the datatype could
>> be pass-by-value rather than a varlena might speed things up quite a
>> bit in some cases.
>
> What cases do you have in mind?

I don't have a specific use case in mind.  However, datumCopy() is
sure to be a lot faster when typByVal is true, and see also the
documentation changes in commit
8472bf7a73487b0535c95e299773b882f7523463.

-- 
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] Optional message to user when terminating/cancelling backend

[Satyanarayana Narlapuram]

+1.

This really helps PostgreSQL Azure service as well. When we are doing the 
upgrades to the service, instead of abruptly terminating the sessions we can 
provide this message.

Thanks,
Satya

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Pavel Stehule
Sent: Monday, June 19, 2017 11:41 AM
To: Daniel Gustafsson 
Cc: PostgreSQL mailing lists 
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling 
backend



2017-06-19 20:24 GMT+02:00 Daniel Gustafsson 
>:
When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped.  This has nagged me in the
past and now it happened to come up again, so I took a stab at this.  The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

  SELECT pg_terminate_backend( [, message]);
  SELECT pg_cancel_backend( [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

  select pg_terminate_backend(, 'server rebooting');

..leads to:

  FATAL:  terminating connection due to administrator command: "server 
rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted.  To keep things simple a small buffer is kept per backend for the
message.  If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

cheers ./daniel

+1

very good idea

Pavel




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

[HACKERS] Re: Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[J Chapman Flack]

On 06/19/2017 11:40 AM, Dilip Kumar wrote:
> ... Artus de benque ... wrote:
>> ...=# UPDATE test_table SET field = rpad('', 2001, 'a') WHERE id = 1;
>
> Seems like in "suppress_redundant_updates_trigger"  we are comparing
> toasted tuple with the new tuple and that is the cause of the bug.

Something still puzzles me about this, though, maybe only because
I don't know enough about TOAST.

The size of 'field' ends up 2001, or just over the threshold where
TOASTing will be attempted at all. The report doesn't mention changing
the strategy from the default EXTENDED, so won't the first thing
attempted be compression? Won't that succeed spectacularly, since the
test string is a single character 2001 times, probably producing
a compressed string a handful of bytes long, well under the threshold,
obviating any need to go further with TOAST pointers?

Is the compression algorithm nondeterministic? Is there some way
that compressing the same 2001*'a' on two occasions would produce
compressed strings that don't match?

What exactly is s_r_u_t() comparing, in the case where the TOASTed
value has been compressed, but not out-of-lined?

-Chap


-- 
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] Optional message to user when terminating/cancelling backend

[Pavel Stehule]

2017-06-19 20:24 GMT+02:00 Daniel Gustafsson :

> When terminating, or cancelling, a backend it’s currently not possible to
> let
> the signalled session know *why* it was dropped.  This has nagged me in the
> past and now it happened to come up again, so I took a stab at this.  The
> attached patch implements the ability to pass an optional text message to
> the
> signalled session which is included in the error message:
>
>   SELECT pg_terminate_backend( [, message]);
>   SELECT pg_cancel_backend( [, message]);
>
> Right now the message is simply appended on the error message, not sure if
> errdetail or errhint would be better? Calling:
>
>   select pg_terminate_backend(, 'server rebooting');
>
> ..leads to:
>
>   FATAL:  terminating connection due to administrator command: "server
> rebooting"
>
> Omitting the message invokes the command just like today.
>
> The message is stored in a new shmem area which is checked when the
> session is
> aborted.  To keep things simple a small buffer is kept per backend for the
> message.  If deemed too costly, keeping a central buffer from which slabs
> are
> allocated can be done (but seemed rather complicated for little gain
> compared
> to the quite moderate memory spend.)
>
> cheers ./daniel
>

+1

very good idea

Pavel


>
>
>
> --
> 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] Preliminary results for proposed new pgindent implementation

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut
>  wrote:
> > On 6/16/17 10:51, Tom Lane wrote:
> >> So I'm back to the position that we ought to stick the indent
> >> code under src/tools/ in our main repo.  Is anyone really
> >> seriously against that?
> >
> > I think it would be better to have it separate.
> 
> +1.

+1.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped.  This has nagged me in the
past and now it happened to come up again, so I took a stab at this.  The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

  SELECT pg_terminate_backend( [, message]);
  SELECT pg_cancel_backend( [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

  select pg_terminate_backend(, 'server rebooting');

..leads to:

  FATAL:  terminating connection due to administrator command: "server 
rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted.  To keep things simple a small buffer is kept per backend for the
message.  If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.) 

cheers ./daniel



terminate_msg_v2.patch
Description: Binary data

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

Re: [HACKERS] INSERT ... ON CONFLICT () SELECT

[Matt Pulver]

On Sun, Jun 18, 2017 at 9:21 PM, Peter Geoghegan  wrote:

> Returning rows with duplicate values seems rather unorthodox.
>

Ok, then option 2 it is.

In summary, this is what I am going to (attempt to) implement for the new
syntax:

INSERT ...
ON CONFLICT (...) DO SELECT
RETURNING ...


   1. Rows that are in conflict are made available to the RETURNING clause.
   In other words, it is like an idempotent "ON CONFLICT DO UPDATE".
   2. Similarly, insertion sets that would cause the error "ON CONFLICT DO
   UPDATE command cannot affect row a second time" if it were an "ON CONFLICT
   DO UPDATE" statement will also cause a similar error for "ON CONFLICT DO
   SELECT". This will prevent duplicate rows from being returned.
   3. Like an "ON CONFLICT DO UPDATE", the returned rows cannot be changed
   by another part of the wCTE, even if no actual insertions occurred.

Unless I have missed anything, I think all other issues have been
adequately addressed. Since there are no red lights, I shall proceed. :)

Best regards,
Matt

Re: [HACKERS] GSoC 2017 weekly progress reports (week 3)

[Shubham Barai]

Hi ,

On 19 June 2017 at 22:49, Alvaro Herrera  wrote:

> Shubham Barai wrote:
> > Project: Explicitly support predicate locks in index AMs besides b-tree
> >
> > Hi,
> >
> >
> > During this week, I continued my work on testing and created my first
> patch
> > for gist index.
>
> Please post it.
>

I have already sent my patch on 16th June. I have also registered it on
commitfest. In case you missed it, please have a look at attached patch.

Regards,
Shubham


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


Predicate-Locking-in-Gist-index_2.patch
Description: Binary data

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

[HACKERS] CREATE SUBSCRIPTION documentation

[Jeff Janes]

https://www.postgresql.org/docs/devel/static/sql-createsubscription.html

Has the note:

See Section 26.2.5.1

for
details on how to configure access control between the subscription and the
publication instance.

But that section seems to describe authentication for physical, not
logical, replication.  Those two no longer use the same access control
methods.  For logical replication, the role has to have replication
attribute or be a superuser, but the role does not need to have replication
listed in the pg_hba.conf.

I think it would be better to instead reference:

https://www.postgresql.org/docs/devel/static/logical-replication-security.html

And on that page, it says:

"The role used for the replication connection must have the REPLICATION
 attribute"

Should also say something like "or be a superuser".  It is bit tedious to
say that everywhere, but the docs are supposed to be a bit tedious.

Cheers,

Jeff

Re: [HACKERS] GSoC 2017 weekly progress reports (week 3)

[Alvaro Herrera]

Shubham Barai wrote:
> Project: Explicitly support predicate locks in index AMs besides b-tree
> 
> Hi,
> 
> 
> During this week, I continued my work on testing and created my first patch
> for gist index.

Please post it.

-- 
Á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] [BUGS] Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:
> > On Mon, Jun 19, 2017 at 11:59 AM, Tom Lane  wrote:
> >> I don't think it's a bug, I think it's an intentional design tradeoff.
> >> To suppress an update in this case, the trigger would have to grovel
> >> through the individual fields and detoast them before comparing.
> >> That would add a lot of cycles, and only seldom add successes.
> >> 
> >> Possibly we should adjust the documentation so that it doesn't imply
> >> that this trigger guarantees to suppress every no-op update.
> 
> > That doesn't sound like a very plausible argument to me.  I don't
> > think that a proposal to add a function named
> > sometimes_suppress_redundant_updates_trigger() would've attracted many
> > votes.
> 
> You'd be wrong.  The entire point of this trigger is to save cycles,
> so having it eat a lot of cycles only to fail is not an improvement.

I suppose that either behavior may be desirable depending on
circumstances.  Maybe it is possible to have each installed trigger be
configurable so that it can select either behavior.  (Maybe use the
trigger argument as a column list, and for each column in the list, do a
full detoast and compare instead of relying on toast pointer equality).

The current behavior seems more convenient in more cases, and so should
remain the default.

But this sounds like an additional feature, not a bug.

-- 
Á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] Decimal64 and Decimal128

[Peter Geoghegan]

On Mon, Jun 19, 2017 at 10:00 AM, Robert Haas  wrote:
> I've never been very happy with the performance of numeric, so I guess
> I'm a bit more optimistic about the chances of doing better.  Aside
> from any computational optimizations, the fact that the datatype could
> be pass-by-value rather than a varlena might speed things up quite a
> bit in some cases.

What cases do you have in mind?


-- 
Peter Geoghegan


-- 
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] [BUGS] Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Artus de benque]

Hi,

It looks like you know what is happening, but I found that I have made an
error in my original assumption: (while the steps to reproduce are still
valid)

The size of the string at which the trigger does not work as expected
varies, depending on the size of the other fields in the row.

The 'limit size' is lower if I set bigger values in another text field in
the same row (and it seems that it is reached when going above 2000 octet
for the texts cells added up).


Sorry if this is noise, and thank you for looking into the bug (or
documentation error).

Regards,

Artus de Benque


Le lun. 19 juin 2017 à 18:20, Tom Lane  a écrit :

> Robert Haas  writes:
> > On Mon, Jun 19, 2017 at 11:59 AM, Tom Lane  wrote:
> >> I don't think it's a bug, I think it's an intentional design tradeoff.
> >> To suppress an update in this case, the trigger would have to grovel
> >> through the individual fields and detoast them before comparing.
> >> That would add a lot of cycles, and only seldom add successes.
> >>
> >> Possibly we should adjust the documentation so that it doesn't imply
> >> that this trigger guarantees to suppress every no-op update.
>
> > That doesn't sound like a very plausible argument to me.  I don't
> > think that a proposal to add a function named
> > sometimes_suppress_redundant_updates_trigger() would've attracted many
> > votes.
>
> You'd be wrong.  The entire point of this trigger is to save cycles,
> so having it eat a lot of cycles only to fail is not an improvement.
>
> regards, tom lane
>

Re: [HACKERS] Decimal64 and Decimal128

[Robert Haas]

On Mon, Jun 19, 2017 at 12:18 PM, Tom Lane  wrote:
> It would be interesting to get some handle on the performance differences
> between decNumber and our existing NUMERIC implementation.  I'm a little
> skeptical that they'd be so enormous as to make this an interesting
> project, but I could be wrong.

I've never been very happy with the performance of numeric, so I guess
I'm a bit more optimistic about the chances of doing better.  Aside
from any computational optimizations, the fact that the datatype could
be pass-by-value rather than a varlena might speed things up quite a
bit in some cases.

On the other hand, the 8-byte version has a decent chance of being
larger on disk than the numeric representation - e.g. $123,456.78 is
only 7 bytes as a short varlena, and won't induce padding out to the
next 8-byte boundary.  And it looks to me like the 4-byte version
can't represent that quantity at all.  That combination of facts seems
like a big problem to me.   A decimal representation that can't handle
more than 7 digits is going to unsuitable for many applications, and
being bigger than our existing numeric on disk for many
commonly-represented values would be awful.

-- 
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] WIP: Data at rest encryption

[Robert Haas]

On Mon, Jun 19, 2017 at 12:30 PM, Robert Haas  wrote:
> On Thu, Jun 15, 2017 at 7:51 PM, Alvaro Herrera
>  wrote:
>> I thought we called it "incremental development".  From the opposite
>> point of view, would you say we should ban use of passphrase-protected
>> SSL key files because the current user interface for them is bad?
>
> I think that we've got a number of features which exist in the tree
> today only because either (a) our standards were lower at the time
> that those features were committed than they are today or (b) we
> didn't realize how much trouble those features were going to create.
> Just because we don't want to hose the people already using those
> features does not mean that we want more features engineered to that
> same quality level.  Obviously, there's room for debate in any
> particular case about how reasonable it is to expect someone who wants
> to implement A to also improve B, and, well, maybe handling thing as
> we do SSL certificates is good enough for this feature, too.  I find
> myself a bit skeptical about that, though.  It preclude as lot of
> things we might want to do.  You're not going to be able to interface
> with some external key management server that way, nor do encryption
> of only part of the database, nor have multiple keys for different
> parts of the database using that kind of setup.
>
> One could argue that can all be added later, but I think there's a
> question about how much that's going to affect the code structure.
> Surely we don't want to install encryption v1 and then find that, by
> not considering key management, we've made it really hard to get to
> v2, and that it basically can't be done without ripping the whole
> implementation out and replacing it.  Maybe the database needs, at
> some rather low level, a concept of whether the encryption key (or an
> encryption key) is available yet, and maybe you get out of considering
> that by deciding you're just going to prompt very early in startup,
> but now when someone comes along and wants to improve things later,
> and they've got to try to go find all of the code that depends on the
> assumption that the key is always available and fix it.  That could be
> pretty annoying to validate.  I think it's better to give at least
> some consideration to these key management questions from the
> beginning, lest we back ourselves into a corner.  Whether or not the
> SSL-passphrase style implementation is above or below the level we'd
> consider a minimally viable feature, it's surely not where we want to
> end up, and we shouldn't do anything that makes it likely that we'll
> get stuck at precisely that point.
>
> Also, practically, I think that type of solution is going to be
> extremely difficult to use for most people.  It means that the
> database can't be started by the system startup scripts; you'll have
> to log into the PG OS user account and launch the database from there.
> IIUC, that means it won't be able to be controlled by things like
> systemd, that just know about start and stop, but not about ask for a
> password in the middle.  Maybe I'm wrong about that, though.  And
> certainly, there will be some users for whom starting the database
> manually and prompting for a password will be good enough, if not
> ideal.  But for people who want to fetch the encryption key from a key
> management server, which I bet is a lot of people, that's not really
> going to be good enough.  I'm not really sure that rushing a first
> patch that "works" for sufficiently small values of "works" is
> actually going

...to move us forward very much.

>> I have no use for data-at-rest encryption myself, but I wouldn't stop
>> development just because the initial design proposal doesn't include
>> top-notch key management.

I agree with that, but there's a difference between "not top-notch"
and "pretty bad".

-- 
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] WIP: Data at rest encryption

[Robert Haas]

On Thu, Jun 15, 2017 at 7:51 PM, Alvaro Herrera
 wrote:
> I thought we called it "incremental development".  From the opposite
> point of view, would you say we should ban use of passphrase-protected
> SSL key files because the current user interface for them is bad?

I think that we've got a number of features which exist in the tree
today only because either (a) our standards were lower at the time
that those features were committed than they are today or (b) we
didn't realize how much trouble those features were going to create.
Just because we don't want to hose the people already using those
features does not mean that we want more features engineered to that
same quality level.  Obviously, there's room for debate in any
particular case about how reasonable it is to expect someone who wants
to implement A to also improve B, and, well, maybe handling thing as
we do SSL certificates is good enough for this feature, too.  I find
myself a bit skeptical about that, though.  It preclude as lot of
things we might want to do.  You're not going to be able to interface
with some external key management server that way, nor do encryption
of only part of the database, nor have multiple keys for different
parts of the database using that kind of setup.

One could argue that can all be added later, but I think there's a
question about how much that's going to affect the code structure.
Surely we don't want to install encryption v1 and then find that, by
not considering key management, we've made it really hard to get to
v2, and that it basically can't be done without ripping the whole
implementation out and replacing it.  Maybe the database needs, at
some rather low level, a concept of whether the encryption key (or an
encryption key) is available yet, and maybe you get out of considering
that by deciding you're just going to prompt very early in startup,
but now when someone comes along and wants to improve things later,
and they've got to try to go find all of the code that depends on the
assumption that the key is always available and fix it.  That could be
pretty annoying to validate.  I think it's better to give at least
some consideration to these key management questions from the
beginning, lest we back ourselves into a corner.  Whether or not the
SSL-passphrase style implementation is above or below the level we'd
consider a minimally viable feature, it's surely not where we want to
end up, and we shouldn't do anything that makes it likely that we'll
get stuck at precisely that point.

Also, practically, I think that type of solution is going to be
extremely difficult to use for most people.  It means that the
database can't be started by the system startup scripts; you'll have
to log into the PG OS user account and launch the database from there.
IIUC, that means it won't be able to be controlled by things like
systemd, that just know about start and stop, but not about ask for a
password in the middle.  Maybe I'm wrong about that, though.  And
certainly, there will be some users for whom starting the database
manually and prompting for a password will be good enough, if not
ideal.  But for people who want to fetch the encryption key from a key
management server, which I bet is a lot of people, that's not really
going to be good enough.  I'm not really sure that rushing a first
patch that "works" for sufficiently small values of "works" is
actually going

> I have no use for data-at-rest encryption myself, but I wouldn't stop
> development just because the initial design proposal doesn't include
> top-notch key management.


-- 
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] [BUGS] Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Tom Lane]

Robert Haas  writes:
> On Mon, Jun 19, 2017 at 11:59 AM, Tom Lane  wrote:
>> I don't think it's a bug, I think it's an intentional design tradeoff.
>> To suppress an update in this case, the trigger would have to grovel
>> through the individual fields and detoast them before comparing.
>> That would add a lot of cycles, and only seldom add successes.
>> 
>> Possibly we should adjust the documentation so that it doesn't imply
>> that this trigger guarantees to suppress every no-op update.

> That doesn't sound like a very plausible argument to me.  I don't
> think that a proposal to add a function named
> sometimes_suppress_redundant_updates_trigger() would've attracted many
> votes.

You'd be wrong.  The entire point of this trigger is to save cycles,
so having it eat a lot of cycles only to fail is not an improvement.

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] Decimal64 and Decimal128

[Tom Lane]

Robert Haas  writes:
> On Sun, Jun 18, 2017 at 6:28 PM, Thomas Munro
>  wrote:
>> I speculate that decNumber in-tree would be the path of least
>> resistance (assuming the "ICU 1.8.1 and later" license[4] would be
>> acceptable -- to my untrained eye it looks rather BSD-ish -- and
>> 20kloc isn't viewed as excessive), and further that a standard
>> compliant version might have some good reasons to be in core rather
>> than in an extension like pgdecimal:

> We should have a very compelling reason for increasing the number of
> such hassles -- and, for me, this feature would not clear that bar.

It would be interesting to get some handle on the performance differences
between decNumber and our existing NUMERIC implementation.  I'm a little
skeptical that they'd be so enormous as to make this an interesting
project, but I could be wrong.

Obviously, the answer could be very different when considering a
mostly-hardware implementation.  But until those are fairly readily
available, it's hard to believe very many people will be excited.

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

[HACKERS] GSoC 2017 weekly progress reports (week 3)

[Shubham Barai]

Project: Explicitly support predicate locks in index AMs besides b-tree

Hi,


During this week, I continued my work on testing and created my first patch
for gist index. I have also started working on the hash index. In summary,
I have done following things in this week.

1) updated tests to check false positives

2) created tests for index scan

3) fixed a small issue with "make check"

4) fixed a small bug in the code by updating the location of
predicatelockpagesplit().

5) read the source code of hash index to understand its implementation

Regards,
Shubham



   Sent with Mailtrack

Re: [HACKERS] [BUGS] Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Robert Haas]

On Mon, Jun 19, 2017 at 11:59 AM, Tom Lane  wrote:
>> Seems like in "suppress_redundant_updates_trigger"  we are comparing
>> toasted tuple with the new tuple and that is the cause of the bug.
>
> I don't think it's a bug, I think it's an intentional design tradeoff.
> To suppress an update in this case, the trigger would have to grovel
> through the individual fields and detoast them before comparing.
> That would add a lot of cycles, and only seldom add successes.
>
> Possibly we should adjust the documentation so that it doesn't imply
> that this trigger guarantees to suppress every no-op update.

That doesn't sound like a very plausible argument to me.  I don't
think that a proposal to add a function named
sometimes_suppress_redundant_updates_trigger() would've attracted many
votes.

-- 
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] Decimal64 and Decimal128

[Robert Haas]

On Sun, Jun 18, 2017 at 6:28 PM, Thomas Munro
 wrote:
> I speculate that decNumber in-tree would be the path of least
> resistance (assuming the "ICU 1.8.1 and later" license[4] would be
> acceptable -- to my untrained eye it looks rather BSD-ish -- and
> 20kloc isn't viewed as excessive), and further that a standard
> compliant version might have some good reasons to be in core rather
> than in an extension like pgdecimal:

I'm not sure it's a good idea to import code under another license,
but leaving that aside, are you volunteering to port every future
change made by the upstream project to our proposed in-tree copy, from
the day the patch is committed until forever?  We've had a few
previous run-ins with this sort of thing: the time zone files, the
regular expression engine, the snowball stuff.  They're not
fantastically high-maintenance but Tom definitely spends some amount
of time on a fairly regular basis updating them and porting over
changes, and they cause hassles with pgindent and so forth as well.
We should have a very compelling reason for increasing the number of
such hassles -- and, for me, this feature would not clear that bar.

I think that if one or both of these libraries are commonly-packaged
things that are reasonably likely to be installable on newer operating
system images using yum/apt-get/port/emerge/whatever then it would be
fine to have a configure switch --with-decfloat or whatever, which
when used includes support for PostgreSQL data types that use the
library.  If those libraries aren't sufficiently commonly-packaged
that this will be realistic option for people, then I vote against
depending on them.  In that case, we could have our own, from-scratch,
clean-room implementation that does not depend on anybody else's code
under some other license, or we could wait and see if they become more
mainstream.

-- 
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] [BUGS] Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Tom Lane]

Dilip Kumar  writes:
> On Mon, Jun 19, 2017 at 5:20 PM, Artus de benque
>  wrote:
>> postgres=# UPDATE test_table SET field = 'hi' WHERE id = 1;
>> UPDATE 0
>> test_db=# UPDATE test_table SET field = rpad('', 2001, 'a') WHERE id = 1;
>> UPDATE 1
>> test_db=# UPDATE test_table SET field = rpad('', 2001, 'a') WHERE id = 1;
>> UPDATE 1 <--- BUG: expected 0, as we ran the same update twice

> Seems like in "suppress_redundant_updates_trigger"  we are comparing
> toasted tuple with the new tuple and that is the cause of the bug.

I don't think it's a bug, I think it's an intentional design tradeoff.
To suppress an update in this case, the trigger would have to grovel
through the individual fields and detoast them before comparing.
That would add a lot of cycles, and only seldom add successes.

Possibly we should adjust the documentation so that it doesn't imply
that this trigger guarantees to suppress every no-op update.

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] Preliminary results for proposed new pgindent implementation

[Robert Haas]

On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut
 wrote:
> On 6/16/17 10:51, Tom Lane wrote:
>> So I'm back to the position that we ought to stick the indent
>> code under src/tools/ in our main repo.  Is anyone really
>> seriously against that?
>
> I think it would be better to have it separate.

+1.

-- 
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] [BUGS] Postgresql bug report - unexpected behavior of suppress_redundant_updates_trigger

[Dilip Kumar]

On Mon, Jun 19, 2017 at 5:20 PM, Artus de benque
 wrote:
> postgres=# UPDATE test_table SET field = 'hi' WHERE id = 1;
> UPDATE 0
> test_db=# UPDATE test_table SET field = rpad('', 2001, 'a') WHERE id = 1;
> UPDATE 1
> test_db=# UPDATE test_table SET field = rpad('', 2001, 'a') WHERE id = 1;
> UPDATE 1 <--- BUG: expected 0, as we ran the same update twice

Seems like in "suppress_redundant_updates_trigger"  we are comparing
toasted tuple with the new tuple and that is the cause of the bug.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files

[Tom Lane]

Dilip Kumar  writes:
> On Mon, Jun 19, 2017 at 6:30 PM, Oleksandr Shulgin
>  wrote:
>> I think can be helpful, though rarely, to be able to send the output of \d*
>> commands to a file.  At the same time it would be nice to see the message on
>> stderr instead of appending to the output file, in case the relation was not
>> found, because of less head-scratching needed to realize the problem.  So
>> I'd vote for changing \dt (and alike) behavior to use stderr.

> +1

> And, we can also change the error message to be more consistent.
> \d says "Did not find any relation named "xxx" ". whereas \dt says "No
> matching relations found".

So, if we're getting into enforcing consistency in describe.c, there's
lots to do.

* listDbRoleSettings does this for a server too old to have the desired
feature:

fprintf(pset.queryFout,
_("No per-database role settings support in this server 
version.\n"));

Just about every other function handles too-old-server like this:

psql_error("The server (version %s) does not support full text 
search.\n",

* listTables and listDbRoleSettings do this for zero matches:

if (PQntuples(res) == 0 && !pset.quiet)
{
if (pattern)
fprintf(pset.queryFout, _("No matching relations 
found.\n"));
else
fprintf(pset.queryFout, _("No relations found.\n"));
}
else
... print the result normally

Note the test on quiet mode, as well as the choice of two messages
depending on whether the command had a pattern argument or not.

Other functions in describe.c mostly follow this pattern:

if (PQntuples(res) == 0)
{
if (!pset.quiet)
psql_error("Did not find any relation named \"%s\".\n",
   pattern);
PQclear(res);
return false;
}

So this has a different behavior in quiet mode, which is to print
*nothing* to queryFout rather than a result table with zero entries.
That's probably bogus.  It will also crash, on some machines, if pattern
is NULL, although no doubt nobody's noticed because there would always be
a match.  (One call site does defend itself against null pattern, and it
uses two messages like the previous example.)

So we've got at least three things to normalize:

* fprintf(pset.queryFout) vs. psql_error()

* message wording inconsistencies

* behavior with -q and no matches.

Anybody want to draft a patch?

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] BUG: pg_dump generates corrupted gzip file in Windows

[Tom Lane]

I wrote:
> That patch looks reasonable to me, will push.

On closer inspection, the patch did contain a bug: it tested for
compression being active with "compression > 0", which is the wrong
thing --- everyplace else in pg_dump tests with "compression != 0".
This is important because Z_DEFAULT_COMPRESSION is -1.  The net
effect is that the patch would have appeared to fix the bug when
writing "-Z1".."-Z9", but not when writing just "-Z".  Possibly
this explains why Kuntal couldn't confirm success at one point
upthread.

Pushed with that correction and some wordsmithing on the comment.

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] psql's \d and \dt are sending their complaints to different output files

[Dilip Kumar]

On Mon, Jun 19, 2017 at 6:30 PM, Oleksandr Shulgin
 wrote:
> I think can be helpful, though rarely, to be able to send the output of \d*
> commands to a file.  At the same time it would be nice to see the message on
> stderr instead of appending to the output file, in case the relation was not
> found, because of less head-scratching needed to realize the problem.  So
> I'd vote for changing \dt (and alike) behavior to use stderr.

+1

And, we can also change the error message to be more consistent.

\d says "Did not find any relation named "xxx" ". whereas \dt says "No
matching relations found".

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] BUG: pg_dump generates corrupted gzip file in Windows

[Tom Lane]

Ashutosh Sharma  writes:
> On Mon, Jun 19, 2017 at 12:25 PM, Amit Kapila  wrote:
>> On Mon, Jun 19, 2017 at 11:42 AM, Ashutosh Sharma  
>> wrote:
>>> On Fri, Mar 24, 2017 at 10:16 PM, Robert Haas  wrote:
 Why not?  I mean, if there's code there to force the output into
 binary mode, does that not work for the -Fc case?  And if it does work
 for the -Fc case, then why doesn't it also work for -Z9?

>>> I have re-verified the patch with the help of my colleague 'Neha
>>> Sharma' on Windows server 2008 R2 machine and the fix looks to be
>>> working fine. With the help of attached patch,

>> Did you intended to attach the patch to this e-mail or are you
>> referring to Kuntal's patch up thread?  If later, then it is better to
>> mention the link of mail which has a patch that you have verified.

> I am referring to Kuntal's patch upthread.  The link for the email
> having the patch is,
> https://www.postgresql.org/message-id/CAGz5QCJPvbBjXAmJuGx1B_41yVCetAJhp7rtaDf7XQGWuB1GSw%40mail.gmail.com

That patch looks reasonable to me, will push.

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] Setting pd_lower in GIN metapage

[Tom Lane]

Amit Kapila  writes:
> On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
>  wrote:
>> What are some arguments against setting pd_lower in the GIN metapage as
>> follows?

> Actually, hash index also has similar code (See _hash_init_metabuffer)
> and I see no harm in doing this at similar other places.

Seems reasonable.

>> How about porting such a change to the back-branches if we do this at all?
>> The reason I'm asking is that a certain backup tool relies on pd_lower
>> values of data pages (disk blocks in relation files that are known to have
>> a valid PageHeaderData) to be correct to discard the portion of every page
>> that supposedly does not contain any useful information.  The assumption
>> doesn't hold in the case of GIN metapage, so any GIN indexes contain
>> corrupted metapage after recovery (metadata overwritten with zeros).

I'm not in favor of back-porting such a change.  Even if we did, it would
only affect subsequently-created indexes not existing ones.  That means
your tool has to cope with an unset pd_lower in any case --- and will for
the foreseeable future, because of pg_upgrade.

I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".

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] drop operator class..using .. left dependency behind.

[Tom Lane]

Rajkumar Raghuwanshi  writes:
> I have observed that even after dropping operator class, not able to drop
> schema containing it. below is a example.

You didn't read the error message closely:

> *postgres=# DROP SCHEMA sch_test;ERROR:  cannot drop schema sch_test
> because other objects depend on itDETAIL:  operator family
> sch_test.custom_opclass_test for access method hash depends on schema
> sch_testHINT:  Use DROP ... CASCADE to drop the dependent objects too.*

The operator class is gone, but the operator family that contained it
still exists.  CREATE OPERATOR CLASS will create a containing family
if you don't specify one, but DROP OPERATOR CLASS won't automatically
remove such a family.  (If it did, it might destroy other operator
classes that had been added to that family later.)

Probably the easiest answer is to use DROP OPERATOR FAMILY instead.

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] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Mon, Jun 19, 2017 at 11:02 AM, Michael Paquier 
wrote:
>
> On Mon, Jun 19, 2017 at 10:58 PM, Fabrízio de Royes Mello
>  wrote:
> > Do you know when the next minor versions will be released? Because
depending
> > of the schedule I'll patch the current customer version because we need
the
> > "pglogical" running stable.
>
> The next round is planned for the 10th of August:
> https://www.postgresql.org/developer/roadmap/
>

I completely forgot this web page ... thanks!

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Michael Paquier]

On Mon, Jun 19, 2017 at 10:58 PM, Fabrízio de Royes Mello
 wrote:
> Do you know when the next minor versions will be released? Because depending
> of the schedule I'll patch the current customer version because we need the
> "pglogical" running stable.

The next round is planned for the 10th of August:
https://www.postgresql.org/developer/roadmap/
-- 
Michael


-- 
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] Setting pd_lower in GIN metapage

[Amit Kapila]

On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
 wrote:
> What are some arguments against setting pd_lower in the GIN metapage as
> follows?
>
> GinMetaPageData *metad = GinPageGetMeta(page);
>
> ((PageHeader) page)->pd_lower =
> ((char *) metad + sizeof(GinMetaPageData)) - (char *) page;
>
> I saw that _bt_initmetapage() does it, so was wondering why doesn't GIN.
>

Actually, hash index also has similar code (See _hash_init_metabuffer)
and I see no harm in doing this at similar other places.  This helps
in compressing the hole in metapage during WAL writing.  I think that
in itself might not be an argument in favor of doing this because
there is only one meta page for index and saving ~7K WAL is not huge
but OTOH I don't see a reason for not doing it.

> How about porting such a change to the back-branches if we do this at all?
>  I couldn't find any discussion in the archives about this.  I read in
> comments that server versions older than 9.4 didn't set pd_lower correctly
> in any of GIN index pages, so relying on pd_lower value of GIN pages is
> unreliable in general.
>
> The reason I'm asking is that a certain backup tool relies on pd_lower
> values of data pages (disk blocks in relation files that are known to have
> a valid PageHeaderData) to be correct to discard the portion of every page
> that supposedly does not contain any useful information.  The assumption
> doesn't hold in the case of GIN metapage, so any GIN indexes contain
> corrupted metapage after recovery (metadata overwritten with zeros).
>

Why can't you do a special check for metapage identification?  It
should be easy to add such a check.  I have seen one of such tools
(pg_filedump) has similar check to skip metapage in certain code
paths.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Directory pg_replslot is not properly cleaned

[Fabrízio de Royes Mello]

On Sun, Jun 18, 2017 at 11:32 PM, Andres Freund  wrote:
>
> Hi,
>
> On 2017-06-07 15:46:45 -0300, Fabrízio de Royes Mello wrote:
> > > >> Just adding Dimitriy to conversation... previous email I provided
was
> > > >wrong.
> > > >>
> > > >
> > > >Does anyone have some thought about this critical issue?
> > > >
> > >
> > > I plan to look into it over the next few days.
> > >
> >
> > Thanks...
>
> As noted in
>
http://archives.postgresql.org/message-id/20170619023014.qx7zjmnkzy3fwpfl%40alap3.anarazel.de
> I've pushed a fix for this.  Sorry for it taking this long.
>

Don't worry... thank you so much.


> The fix will be included in the next set of minor releases.
>

Do you know when the next minor versions will be released? Because
depending of the schedule I'll patch the current customer version because
we need the "pglogical" running stable.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] RLS policy not getting honer while pg_dump on declarative partition

[Stephen Frost]

Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/06/17 9:20, Stephen Frost wrote:
> > I think we could certainly consider if this behavior is desirable in a
> > system which includes partitioning instead of inheritance,
> 
> Do we want CREATE POLICY foo ON parent creating the policy objects for all
> the partitions?  That is, cascade the policy object definition?

That seems to be what the OP is suggesting we should have.  I'm
certainly not convinced that we actually do want that though.  There are
clear reasons why it doesn't make sense for inheritance that aren't an
issue for partitions (for one thing, we don't have to worry about the
visible columns being different between the partitioned table and the
partitions), but that doesn't necessairly mean we want to make this
different for partitions vs. inheritance.

In any case though, I do tend to feel that it's rather too late to
consider changing things for PG10 in this area, even if we all felt that
it was the right/correct thing to do, which isn't clear.

> > but if we
> > wish to do so then I think we should be considering if the GRANT system
> > should also be changed as I do feel the two should be consistent.
> 
> IIUC, you are saying here that GRANT should be taught to cascade the
> permission grant/revokes to partitions.

What I'm saying here is that the way GRANT works and the way policies
are applied to partitioned tables should be consistent with each other.

> Also, the somewhat related nearby discussion about dumping the partition
> data through the root parent will perhaps have to think about some of
> these things.  Dumping data through the root table will take care of the
> problem that Rushabh is complaining about, because only rows visible per
> the parent's policies would be dumped.  Of course then the the set of rows
> dumped will be different from what it is today, because one would expect
> that a different set of policies would get applied - the root table's
> policies when dumping data through it vs. the individual partitions'
> policies when dumping data per partition.

While somewhat related, I don't think we should allow concerns about
pg_dump to drive what we're doing in the backend.  If pg_dump isn't
smart enough to allow different ways to dump the data out given the
considerations for what the backend supports/does, then we should add
new features to pg_dump, not reconsider what we're doing in the backend.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] drop operator class..using .. left dependency behind.

[Rajkumar Raghuwanshi]

Hi,

I have observed that even after dropping operator class, not able to drop
schema containing it. below is a example.

postgres=# CREATE SCHEMA sch_test;
CREATE SCHEMA
postgres=# SET search_path TO 'sch_test';
SET
postgres=# CREATE OR REPLACE FUNCTION sch_test.dummy_hashint4_39779(a int4)
RETURNS int4 AS $$ BEGIN RETURN a; END; $$ LANGUAGE 'plpgsql' IMMUTABLE;
CREATE FUNCTION
postgres=# CREATE OPERATOR CLASS sch_test.custom_opclass_test FOR TYPE int4
USING HASH AS OPERATOR 1 = , FUNCTION 1 sch_test.dummy_hashint4_39779(a
int4);
CREATE OPERATOR CLASS

*postgres=# DROP OPERATOR CLASS sch_test.custom_opclass_test USING
HASH;DROP OPERATOR CLASS*
postgres=# DROP FUNCTION sch_test.dummy_hashint4_39779(a int4);
DROP FUNCTION
postgres=# RESET search_path;
RESET




*postgres=# DROP SCHEMA sch_test;ERROR:  cannot drop schema sch_test
because other objects depend on itDETAIL:  operator family
sch_test.custom_opclass_test for access method hash depends on schema
sch_testHINT:  Use DROP ... CASCADE to drop the dependent objects too.*
when investigated found, entry still present in pg_opfamily.

postgres=# select * from pg_opfamily where opfname like
'custom_opclass_test%';
 opfmethod |   opfname   | opfnamespace | opfowner
---+-+--+--
   405 | custom_opclass_test |16409 |   10
(1 row)

Is this expected behaviour??

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

[HACKERS] psql's \d and \dt are sending their complaints to different output files

[Oleksandr Shulgin]

Hello Hackers,

I wonder if it is intentional that \d complains on stderr if it cannot find
relations to match, but \dt prints the message to the current output file?

postgres=# \d xxx
Did not find any relation named "xxx".
postgres=# \dt xxx
No matching relations found.

I've noticed the difference exactly because my output was
(accidentally) redirected to a file and I didn't see the complaint from the
2nd backslash command.

By browsing and grepping src/bin/psql/describe.c I can see that
psql_error() (used in \d) is prevalent, as opposed to fprintf() to
pset.queryFout (used in \dt), but then it's a question if it should be
treated as an error or not.

I think can be helpful, though rarely, to be able to send the output of \d*
commands to a file.  At the same time it would be nice to see the message
on stderr instead of appending to the output file, in case the relation was
not found, because of less head-scratching needed to realize the problem.
So I'd vote for changing \dt (and alike) behavior to use stderr.

Regards,
--
Alex

Re: [HACKERS] Making server name part of the startup message

[Satyanarayana Narlapuram]

-Original Message-
From: Andres Freund [mailto:and...@anarazel.de] 
Sent: Friday, June 16, 2017 10:48 AM
To: Tom Lane 
Cc: Satyanarayana Narlapuram ; 
pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Making server name part of the startup message

On 2017-06-15 09:43:13 -0400, Tom Lane wrote:
> Satyanarayana Narlapuram  writes:
> > As a cloud service, Azure Database for PostgreSQL uses a gateway proxy to 
> > route connections to a node hosting the actual server. To do that, the 
> > proxy needs to know the name of the server it tries to locate. As a 
> > work-around we currently overload the username parameter to pass in the 
> > server name using username@servername convention. It is purely a convention 
> > that our customers need to follow and understand. We would like to extend 
> > the PgSQL connection protocol to add an optional parameter for the server 
> > name to help with this scenario.
> 
> We don't actually have any concept of a server name at the moment, and 
> it isn't very clear what introducing that concept would buy.
> Please explain.

cluster_name could be what's meant?

Andres, thank you! It is database cluster name as you mentioned.

- 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] ASOF join

[Konstantin Knizhnik]

On 16.06.2017 19:07, David Fetter wrote:

On Fri, Jun 16, 2017 at 11:51:34AM +1200, Thomas Munro wrote:

On Fri, Jun 16, 2017 at 4:20 AM, Konstantin Knizhnik
 wrote:

I wonder if there were some discussion/attempts to add ASOF join to Postgres
(sorry, may be there is better term for it, I am refereeing KDB definition:
http://code.kx.com/wiki/Reference/aj ).

Interesting idea.  Also in Pandas:

http://pandas.pydata.org/pandas-docs/version/0.19.0/generated/pandas.merge_asof.html#pandas.merge_asof


I attached simple patch adding ASOF join to Postgres. Right now it 
support only outer join and requires USING clause (consequently it is 
not possible to join two tables which joi keys has different names. May 
be it is also possible to support ON clause with condition written like 
o.k1 = i.k2 AND o.k2 = i.k2 AND ... AND o.kN >= i.kN
But such notation can be confusing, because join result includes only 
one matching inner record with kN smaller or equal than kN of outer 
record and not all such records.

As alternative we can add specia

If people fin such construction really useful, I will continue work on it.




I suppose you could write a function that pulls tuples out of a bunch
of cursors and zips them together like this, as a kind of hand-coded
special merge join "except that we match on nearest key rather than
equal keys" (as they put it).

I've written code like this before in a trading context, where we
called that 'previous tick interpolation', and in a scientific context
where other kinds of interpolation were called for (so not really
matching a tuple but synthesising one if no exact match).  If you view
the former case as a kind of degenerate case of interpolation then it
doesn't feel like a "join" as we know it, but clearly it is.  I had
never considered before that such things might belong inside the
database as a kind of join operator.

If you turn your head sideways, it's very similar to the range merge
join Jeff Davis proposed.  https://commitfest.postgresql.org/14/1106/


May be, but I do not understand how to limit result to contain exactly 
one (last) inner tuple for each outer tuple.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 482a3dd..f7a8f38 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1324,6 +1324,9 @@ get_jointype_name(JoinType jointype)
 		case JOIN_FULL:
 			return "FULL";
 
+		case JOIN_ASOF:
+			return "ASOF";
+
 		default:
 			/* Shouldn't come here, but protect from buggy code. */
 			elog(ERROR, "unsupported join type %d", jointype);
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 080cb0a..54cf6c1 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4073,7 +4073,7 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 	 * Constructing queries representing SEMI and ANTI joins is hard, hence
 	 * not considered right now.
 	 */
-	if (jointype != JOIN_INNER && jointype != JOIN_LEFT &&
+	if (jointype != JOIN_INNER && jointype != JOIN_LEFT && jointype != JOIN_ASOF && 
 		jointype != JOIN_RIGHT && jointype != JOIN_FULL)
 		return false;
 
@@ -4211,6 +4211,7 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 			break;
 
 		case JOIN_LEFT:
+		case JOIN_ASOF:
 			fpinfo->joinclauses = list_concat(fpinfo->joinclauses,
 		  list_copy(fpinfo_i->remote_conds));
 			fpinfo->remote_conds = list_concat(fpinfo->remote_conds,
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 211e4c3..fd3be8c 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -514,6 +514,9 @@ TABLE [ ONLY ] table_name [ * ]
  
   CROSS JOIN
  
+ 
+  ASOF [ OUTER ] JOIN
+ 
 
 
 For the INNER and OUTER join types, a
@@ -523,7 +526,9 @@ TABLE [ ONLY ] table_name [ * ]
 USING (join_column [, ...]).
 See below for the meaning.  For CROSS JOIN,
-none of these clauses can appear.
+none of these clauses can appear. For ASOF join type, a
+join condition must be USING (join_column [, ...]).

 

@@ -571,6 +576,32 @@ TABLE [ ONLY ] table_name [ * ]
 on the right), plus one row for each unmatched right-hand row
 (extended with nulls on the left).

+
+   ASOF OUTER JOIN is similar to LEFT OUTER JOIN but it accepts only
+USING (join_column_1 [, ...], join_column_N) clause
+where last joined column join_column_N is expected to be timestamp 
+(but actually can have any comparable type) and outer tuple is matched with only one inner tuple with the same values of 1..N-1 
+join columns and with largest value of join_column_N which is smaller or equal than 
+ 

[HACKERS] Rules on table partitions

[Dean Rasheed]

Currently we allow rules to be defined on table partitions, but these
rules only fire when the partition is accessed directly, not when it
is accessed via the parent:

CREATE TABLE t1(a int, b int) PARTITION BY RANGE(a);
CREATE TABLE t1_p PARTITION OF t1 FOR VALUES FROM (1) TO (10);
INSERT INTO t1 VALUES (1,101), (2,201);

CREATE TABLE t1_p_log(a int, b int, d date);
CREATE RULE t1_p_log_rule AS ON UPDATE TO t1_p
  DO ALSO INSERT INTO t1_p_log VALUES(old.a, old.b, now());

UPDATE t1 SET b=b+1 WHERE a=1;
UPDATE t1_p SET b=b+1 WHERE a=2;

SELECT * FROM t1_p_log;

 a |  b  | d
---+-+
 2 | 201 | 2017-06-19
(1 row)


I'd regard that as a bug, especially since this kind of thing would
have worked with old-style user-defined partitioning. Perhaps we
should explicitly forbid this for now -- i.e., raise a "not supported"
error when attempting to add a rule to a partition, or attach a table
with rules to a partitioned table.

Personally, I wouldn't regard adding proper support for rules on
partitions as a high priority, so I'd be OK with it remaining
unsupported unless someone cares enough to implement it, but that
seems preferable to leaving it partially working in this way.

Also, as things stand, it is possible to do the following:

CREATE TABLE t2(a int, b int) PARTITION BY RANGE(a);
CREATE TABLE t2_p PARTITION OF t2 FOR VALUES FROM (1) TO (10);
CREATE RULE "_RETURN" AS ON SELECT TO t2_p DO INSTEAD SELECT * FROM t2;

which results in the partition becoming a view that selects from the
parent, which surely ought to be forbidden.

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] RLS policy not getting honer while pg_dump on declarative partition

[Amit Langote]

On 2017/06/17 9:20, Stephen Frost wrote:
> Greetings,
> 
> * Rushabh Lathia (rushabh.lat...@gmail.com) wrote:
>> While doing some testing I noticed that RLS policy not getting honer
>> while pg_dump on declarative partition.
>>
>> I can understand that while doing SELECT on individual child
>> table, policy of parent is not getting applied. But is this desirable
>> behaviour? I think for partitions, any policy on the root table should
>> get redirect to the child, thoughts?
>>
>> If current behaviour is desirable then atleast we should document this.
> 
> The current behaviour matches how the GRANT system works, unless it's
> been changed as part of the partitioning patches, we don't check the
> privileges on tthe parent to see if an individual has access to the
> child.

The partitioning patch itself (either f0e44751d71 or any subsequent
patches) didn't change any of how that works.  One would get the same
behavior as one would with inheritance, as described in the inheritance
section:

https://www.postgresql.org/docs/devel/static/ddl-inherit.html

"Inherited queries perform access permission checks on the parent table
only. Thus, for example, granting UPDATE permission on the cities table
implies permission to update rows in the capitals table as well, when they
are accessed through cities. This preserves the appearance that the data
is (also) in the parent table. But the capitals table could not be updated
directly without an additional grant. In a similar way, the parent table's
row security policies (see Section 5.7) are applied to rows coming from
child tables during an inherited query. A child table's policies, if any,
are applied only when it is the table explicitly named in the query; and
in that case, any policies attached to its parent(s) are ignored."

> I think we could certainly consider if this behavior is desirable in a
> system which includes partitioning instead of inheritance,

Do we want CREATE POLICY foo ON parent creating the policy objects for all
the partitions?  That is, cascade the policy object definition?

> but if we
> wish to do so then I think we should be considering if the GRANT system
> should also be changed as I do feel the two should be consistent.

IIUC, you are saying here that GRANT should be taught to cascade the
permission grant/revokes to partitions.


Also, the somewhat related nearby discussion about dumping the partition
data through the root parent will perhaps have to think about some of
these things.  Dumping data through the root table will take care of the
problem that Rushabh is complaining about, because only rows visible per
the parent's policies would be dumped.  Of course then the the set of rows
dumped will be different from what it is today, because one would expect
that a different set of policies would get applied - the root table's
policies when dumping data through it vs. the individual partitions'
policies when dumping data per partition.

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] Refreshing subscription relation state inside a transaction block

[Masahiko Sawada]

On Thu, Jun 15, 2017 at 11:49 PM, Petr Jelinek
 wrote:
> On 15/06/17 15:52, Peter Eisentraut wrote:
>> On 6/15/17 02:41, Petr Jelinek wrote:
>>> Hmm, forcibly stopping currently running table sync is not what was
>>> intended, I'll have to look into it. We should not be forcibly stopping
>>> anything except the main apply worker during drop subscription (and we
>>> do that only because we can't drop the remote replication slot otherwise).
>>
>> The change being complained about was specifically to address the
>> problem described in the commit message:
>>
>> Stop table sync workers when subscription relation entry is removed
>>
>> When a table sync worker is in waiting state and the subscription table
>> entry is removed because of a concurrent subscription refresh, the
>> worker could be left orphaned.  To avoid that, explicitly stop the
>> worker when the pg_subscription_rel entry is removed.
>>
>>
>> Maybe that wasn't the best solution.  Alternatively, the tablesync
>> worker has to check itself whether the subscription relation entry has
>> disappeared, or we need a post-commit check to remove orphaned workers.
>>
>
> Ah I missed that commit/discussion, otherwise I would have probably
> complained. I don't like killing workers in the DDL command, as I said
> the only reason we do it in DropSubscription is to free the slot for
> dropping.
>
> I think that either of the options you suggested now would be better.
> We'll need that for stopping the tablesync which is in progress during
> DropSubscription as well as those will currently still keep running. I
> guess we could simply just register syscache callback, the only problem
> with that is we'd need to AcceptInvalidationMessages regularly while we
> do the COPY which is not exactly free so maybe killing at the end of
> transaction would be better (both for refresh and drop)?
>

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Regards,

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


check_subscription_rel_state_after_copy.patch
Description: Binary data

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

Re: [HACKERS] transition table behavior with inheritance appears broken

[Amit Langote]

On 2017/06/19 7:59, Andrew Gierth wrote:
>> "Andrew" == Andrew Gierth  writes:
> > (Any preferences for whether it should be one commit or 3 separate ones?)

For my 2c, it would be nice to keep at least the inheritance one (or all
of them actually) separate.

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] Phantom segment upon promotion causing troubles.

[Andres Freund]

Hi,

On 2017-06-19 00:30:26 -0700, Andres Freund wrote:
> There seems to be a larger question ehre though: Why does
> XLogFileReadAnyTLI() probe all timelines even if they weren't a parent
> at that period?  That seems like a bad idea, especially in more
> complicated scenarios where some precursor timeline might live for
> longer than it was a parent?  ISTM XLogFileReadAnyTLI() should check
> which timeline a segment ought to come from, based on the historY?

One thing that I blamed first, before debunking it, is that after
promotion we do:

/*
 * Preallocate additional log files, if wanted.
 */
PreallocXlogFiles(EndOfLog);

where EndOfLog points to the last replayed record, rather than last
record(s).  I think that's currently harmless, but it's certainly
fragile.  Given the uselessness of PreallocXlogFiles() calls, I'm
inclined to just remove it here...

- Andres


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

[HACKERS] Phantom segment upon promotion causing troubles.

[Andres Freund]

Hi,

Greg Burek from Heroku (CCed) reported a weird issue on IM, that was
weird enough to be interesting.  What he'd observed was that he promoted
some PITR standby, and early clones of that node work, but later clones
did not, failing to read some segment.

The problems turns out to be the following:  When a node is promoted at
a segment boundary, just after an XLOG_SWITCH record we'll have
EndOfLog = EndRecPtr;
pointing to the *beginning* of the next segment, as XLOG_SWITCH records
are treated as using the whole segment.  After creating the
END_OF_RECOVERY record (or checkpoint), we'll do:

if (ArchiveRecoveryRequested)
{
/*
 * We switched to a new timeline. Clean up segments on the old
 * timeline.
 *
 * If there are any higher-numbered segments on the old 
timeline,
 * remove them. They might contain valid WAL, but they might 
also be
 * pre-allocated files containing garbage. In any case, they 
are not
 * part of the new timeline's history so we don't need them.
 */
RemoveNonParentXlogFiles(EndOfLog, ThisTimeLineID);

note that this uses EndOfLog, pointing to ab/cd00 (i.e. the
beginning of a record). RemoveNonParentXlogFiles calls
RemoveNonParentXlogFiles() which in turn uses RemoveXlogFile() to remove
superflous files.  That's where the fun begins.

static void
RemoveXlogFile(const char *segname, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
{
XLogSegNo   endlogSegNo;
XLogSegNo   recycleSegNo;
...
#define XLByteToPrevSeg(xlrp, logSegNo) \
logSegNo = ((xlrp) - 1) / XLogSegSize
...
XLByteToPrevSeg(endptr, endlogSegNo);
if (PriorRedoPtr == InvalidXLogRecPtr)
recycleSegNo = endlogSegNo + 10;
else
recycleSegNo = XLOGfileslop(PriorRedoPtr);
...
InstallXLogFileSegment(, path,
   true, recycleSegNo, 
true))
...

So what happens here is that we're calling InstallXLogFileSegment() to
remove superflous xlog files (e.g. because they're before the recovery
target, because restore command ran before the trigger file was detected
or because walsender received them).  But because endptr = ab/cd00,
the use of XLByteToPrevSeg() means InstallXLogFileSegment() will be
called with the *previous* segment's segment number.

That in turn will lead to InstallXLogFileSegment() installing the
to-be-removed segment into the current timeline, but into a segment from
one *before* the creation of new timeline, for the purpose of recycling
the segment.  I'll call this the "phantom" segment, which has no
meaningful content and lives on a timeline which does not yet exist.

As there's no .ready file created for that segment, and we'll never
actually write to it, it'll initially just sit around. Not visible for
archiving, and normally unused by wal streaming.  But that changes at
later checkpoints, because, via RemoveOldXlogFiles()'s
XLogArchiveCheckDone() checks we:
/*
 * XLogArchiveCheckDone
 *
...
 * If .done exists, then return true; else if .ready exists,
 * then return false; else create .ready and return false.
 *
 * The reason we do things this way is so that if the original attempt to
 * create .ready fails, we'll retry during subsequent checkpoints.

So we'll at some later point create a .ready for the above created
phantom segment. Which then will get archived.


At that point we're in trouble.  If any standbys of that promoted node
catch up after that fact (or new ones are created from older base
backups), after the phantom segment has been archived, and
restore_command is set, recovery will fail.  The reason for that is that
one commonly will have recovery_target_timeline = latest (or the new
timeline) set.  And XLogFileReadAnyTLI() is pretty simplistic. When
restoring a segment it'll simply probe all timelines, starting from the
newest. Which means that, once archived, our phantom segment will "hide"
the actual segment from the source timeline.  Because it's not parseable
(it's at a different segment, thus parsing decide it's unusable),
recovery will hang at that point.

Which means quick standbys catch up, slow ones are "dead". It's
"fixable" by creating a restore_command which filters that phantom
segment, or deleting the segment from the archive.


The minimal fix here is presumably not to use XLByteToPrevSeg() in
RemoveXlogFile(), but XLByteToSeg(). I don't quite see what purpose it
serves here - I don't think it's ever needed.  Normally it's harmless
because InstallXLogFileSegment() checks where it could install the file
to, but that doesn't work around timeline bumps, triggering the problem
at hand.  This seems to be very longstanding behaviour, I'm not sure
where it's originating from (hard to track due to code movement).


There seems to be a larger question ehre