Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Thu, Aug 3, 2017 at 11:26 PM, Daniel Gustafsson wrote: >> On 03 Aug 2017, at 19:27, Michael Paquier wrote: >> There were no APIs to get the TLS finish message last time I looked at OSX >> stuff, which mattered for tls-unique. It would be nice if we could get one. > > Yeah, AFAICT there is no API for that. Perhaps my last words were confusing. I meant that it would be nice to get at least one type of channel binding working. -- 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] Quorum commit for multiple synchronous replication.
On Fri, Jul 28, 2017 at 2:24 PM, Noah Misch wrote: > On Thu, Apr 06, 2017 at 08:55:37AM +0200, Petr Jelinek wrote: >> On 06/04/17 03:51, Noah Misch wrote: >> > On Thu, Apr 06, 2017 at 12:48:56AM +0900, Fujii Masao wrote: >> >> On Wed, Apr 5, 2017 at 3:45 PM, Noah Misch wrote: >> >>> On Mon, Dec 19, 2016 at 09:49:58PM +0900, Fujii Masao wrote: >> Regarding this feature, there are some loose ends. We should work on >> and complete them until the release. >> >> (1) >> Which synchronous replication method, priority or quorum, should be >> chosen when neither FIRST nor ANY is specified in s_s_names? Right now, >> a priority-based sync replication is chosen for keeping backward >> compatibility. However some hackers argued to change this decision >> so that a quorum commit is chosen because they think that most users >> prefer to a quorum. > >> >> The items (1) and (3) are not bugs. So I don't think that they need to be >> >> resolved before the beta release. After the feature freeze, many users >> >> will try and play with many new features including quorum-based syncrep. >> >> Then if many of them complain about (1) and (3), we can change the code >> >> at that timing. So we need more time that users can try the feature. >> > >> > I've moved (1) to a new section for things to revisit during beta. If >> > someone >> > feels strongly that the current behavior is Wrong and must change, speak >> > up as >> > soon as you reach that conclusion. Absent such arguments, the behavior >> > won't >> > change. >> > >> >> I was one of the people who said in original thread that I think the >> default behavior should change to quorum and I am still of that opinion. > > This item appears under "decisions to recheck mid-beta". If anyone is going > to push for a change here, now is the time. It has been 1 week since the previous mail. I though that there were others argued to change the behavior of old-style setting so that a quorum commit is chosen. If nobody is going to push for a change we can live with the current behavior? 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] Unused variable scanned_tuples in LVRelStats
On Fri, Aug 4, 2017 at 3:01 AM, Robert Haas wrote: > On Thu, Aug 3, 2017 at 1:10 AM, Masahiko Sawada wrote: >> So we can remove scanned_tuples from LVRelStats struct and change the >> variable name num_tuples to scanned_tuples. Attached updated patch. > > On second thought, I think we should just leave this alone. > scanned_tuples is closely tied to tupcount_pages, so it's a little > confusing to pull one out and not the other. And we can't pull > tupcount_pages out of LVRelStats because lazy_cleanup_index needs it. > The current situation isn't doing any harm, so I'm not seeing much > point in changing it. > Hmm, since I agree the current situation isn't doing any harm actually I don't push hard for this but I'd like to still propose at least the original patch that fixes an inconsistent in the code. 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] PostgreSQL not setting OpenSSL session id context?
Shay Rojansky writes: > I tested the patch. Thanks! > Doing SSL_CTX_set_session_cache_mode(context, SSL_SESS_CACHE_OFF) doesn't > have any effect whatsoever - I still have the same issue (session id > context uninitialized). I suspect session caching is an entirely different > feature from session tickets/RFC5077 (although it might still be a good > idea to disable). Right, we expected that that would have no visible effect, because there is no way to cache sessions in Postgres anyway. The main point, if I understand Heikki's concern correctly, is that this might save some amount of low-level overhead from clients trying to cache connections. > Doing SSL_CTX_set_options(context, SSL_OP_NO_TICKET) indeed resolves the > issue, as expected. Excellent. I'll push this patch tomorrow sometime (too late/tired right now). > As I wrote above, I'd remove the #ifdef and execute it always. The reason I put the #ifdef in is that according to my research the SSL_OP_NO_TICKET symbol was introduced in openssl 0.9.8f, while we claim to support back to 0.9.8. I'd be the first to say that you're nuts if you're running openssl versions that old; but this patch is not something to move the compatibility goalposts for when it only takes an #ifdef to avoid breaking older versions. (I need to check how far back SSL_SESS_CACHE_OFF goes ... we might need an #ifdef for that too.) 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] PostgreSQL not setting OpenSSL session id context?
On 2017-08-04 07:22:42 +0300, Shay Rojansky wrote: > I'm still not convinced of the risk/problem of simply setting the session > id context as I explained above (rather than disabling the optimization), > but of course either solution resolves my problem. How would that do anything? Each backend has it's own local memory. I.e. any cache state that openssl would maintain wouldn't be useful. If you want to take advantage of features around this you really need to cache tickets in shared memory... Greetings, Andres Freund -- 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] PostgreSQL not setting OpenSSL session id context?
I tested the patch. Doing SSL_CTX_set_session_cache_mode(context, SSL_SESS_CACHE_OFF) doesn't have any effect whatsoever - I still have the same issue (session id context uninitialized). I suspect session caching is an entirely different feature from session tickets/RFC5077 (although it might still be a good idea to disable). Doing SSL_CTX_set_options(context, SSL_OP_NO_TICKET) indeed resolves the issue, as expected. As I wrote above, I'd remove the #ifdef and execute it always. I'm still not convinced of the risk/problem of simply setting the session id context as I explained above (rather than disabling the optimization), but of course either solution resolves my problem.
Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server
On Fri, Aug 4, 2017 at 10:48 AM, Stephen Frost wrote: > Michael, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Wed, Aug 2, 2017 at 7:58 PM, Stephen Frost wrote: >> > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> >> Do you need a back-patchable version for 9.6? I could get one out of >> >> my pocket if necessary. >> > >> > I was just trying to find a bit of time to generate exactly that- if >> > you have a couple spare cycles, it would certainly help. >> >> OK, here you go. Even if archive_mode = always has been introduced in >> 9.5, but non-exclusive mode is a 9.6 feature, so here are patches down >> to this version. I am pretty satisfied by this, and I included all the >> comments and error message corrections reviewed up to now. I noticed >> some incorrect comments, doc typos and an incorrect indentation as >> well for the WARNING reported to the user when waiting for the >> archiving. > > Thanks much for this. I've looked over it some and it looks pretty good > to me. I'm going to play with it a bit more and, barring issues, will > push them tomorrow morning. > Thank you for the patches, Michael-san! It looks good to me too. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgsql 10: hash indexes testing
On Wed, Aug 2, 2017 at 9:04 PM, Robert Haas wrote: > On Wed, Jul 12, 2017 at 1:10 AM, Amit Kapila wrote: Yes, I also think the same idea can be used, in fact, I have mentioned it [1] as soon as you have committed that patch. Do we want to do anything at this stage for PG-10? I don't think we should attempt something this late unless people feel this is a show-stopper issue for usage of hash indexes. If required, I think a separate function can be provided to allow users to perform squeeze operation. >>> >>> Sorry, I have no idea how critical this squeeze thing is for the >>> newfangled hash indexes, so I cannot comment on that. Does this make >>> the indexes unusable in some way under some circumstances? >> >> It seems so. Basically, in the case of a large number of duplicates, >> we hit the maximum number of overflow pages. There is a theoretical >> possibility of hitting it but it could also happen that we are not >> free the existing unused overflow pages due to which it keeps on >> growing and hit the limit. I have requested up thread to verify if >> that is happening in this case and I am still waiting for same. The >> squeeze operation does free such unused overflow pages after cleaning >> them. As this is a costly operation and needs a cleanup lock, so we >> currently perform it only during Vacuum and next split from the bucket >> which can have redundant overflow pages. > > Oops. It was rather short-sighted of us not to increase > HASH_MAX_BITMAPS when we bumped HASH_VERSION. Actually removing that > limit is hard, but we could have easily bumped it for 128 to say 1024 > without (I think) causing any problem, which would have given us quite > a bit of headroom here. Yes, that sounds sensible, but I think it will just delay the problem to happen. I think here the actual problem is that we are not able to perform squeeze operation often enough that it frees the overflow pages. Currently, we try to perform the squeeze only at the start of next split of the bucket or during vacuum. The reason for doing it that way was that squeeze operation needs cleanup lock and we already have that during the start of split and vacuum. Now, to solve it I have already speculated few ways above [1] and among those, it is feasible to either do this at end of split which can have performance implications in some work loads, but will work fine for the case reported in this thread and another is to some way (like we do for Brin index in commit 7526e10224f0792201e99631567bbe44492bbde4) trigger vacuum. I think we can fix it in one of above ways and increase the value of HASH_MAX_BITMAPS as well. What do you say? > I suppose we could still try to jam that > change in before beta3 (bumping HASH_VERSION again) but that might be > asking for trouble. > I am not sure if we have any other option if we decide to increase the value of HASH_MAX_BITMAPS. BTW, do you think some users will rely on hash index built on some of the beta version? Note - AP has off list shared the data dump and we (Ashutosh Sharma and me) are able to reproduce the problem and we could see that if we force vacuum via the debugger, then it is able to free overflow pages. The exact numbers are not available at this stage as the test is not complete. [1] - https://www.postgresql.org/message-id/CAA4eK1KKq80BYOc%2BmcmHcQzV0Mcs3AHGjEEf--TnLaJbkeTgmg%40mail.gmail.com -- 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] Update comments in nodeModifyTable.c
On 2017/08/04 1:52, Robert Haas wrote: On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita wrote: I updated the patch that way. Attached is a new version of the patch. Committed. Thanks again. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/04 10:00, Amit Langote wrote: On 2017/08/04 1:06, Robert Haas wrote: On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita wrote: On 2017/08/03 17:12, Amit Langote wrote: Attached updated patches. Thanks for the patch! That looks good to me. Committed with some comment changes. Thanks a lot. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] POC: Cache data in GetSnapshotData()
On 3 Aug 2017 2:16 am, "Andres Freund" wrote: Hi Andres thanks for detailed review. I agree with all of the comments. I am going for a vacation. Once I come back I will fix those comments and will submit a new patch.
Re: [HACKERS] AlterUserStmt anmd RoleSpec rules in grammar.y
On 7/31/17 20:42, Peter Eisentraut wrote: > That looks like a bug to me. ALTER USER also does not support the IN > DATABASE clause, so the code deviation might have started there already. > > I propose the attached patch to clean this up. > > For backpatching, I could develop some less invasive versions. done and done -- 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] pg_stop_backup(wait_for_archive := true) on standby server
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Thu, Aug 3, 2017 at 4:29 AM, Stephen Frost wrote: > > I'll provide another update tomorrow. Hopefully Michael is able to produce > > a 9.6 patch, otherwise I'll do it. > > I have sent an updated version of the patch, with something that can > be used for 9.6 as well. It would be nice to get something into the > next set of minor releases. Thanks for the patches. I'm planning to push them tomorrow morning after a bit more review and testing. I'll provide an update tomorrow. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Wed, Aug 2, 2017 at 7:58 PM, Stephen Frost wrote: > > * Michael Paquier (michael.paqu...@gmail.com) wrote: > >> Do you need a back-patchable version for 9.6? I could get one out of > >> my pocket if necessary. > > > > I was just trying to find a bit of time to generate exactly that- if > > you have a couple spare cycles, it would certainly help. > > OK, here you go. Even if archive_mode = always has been introduced in > 9.5, but non-exclusive mode is a 9.6 feature, so here are patches down > to this version. I am pretty satisfied by this, and I included all the > comments and error message corrections reviewed up to now. I noticed > some incorrect comments, doc typos and an incorrect indentation as > well for the WARNING reported to the user when waiting for the > archiving. Thanks much for this. I've looked over it some and it looks pretty good to me. I'm going to play with it a bit more and, barring issues, will push them tomorrow morning. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump does not handle indirectly-granted permissions properly
Tom, all,
* Stephen Frost (sfr...@snowman.net) wrote:
> This needs more cleanup, testing, and comments explaining why we're
> doing this (and then perhaps comments, somewhere.. in the backend ACL
> code that explains that the ordering needs to be preserved), but the
> basic idea seems sound to me and the case you presented does work with
> this patch (for me, at least) whereas what's in master didn't.
Alright, here's an updated patch which cleans things up a bit and adds
comments to explain what's going on. I also updated the comments in
acl.h to explain that ordering actually does matter.
I've tried a bit to break the ordering in the backend a bit but there
could probably be more effort put into that, if I'm being honest.
Still, this definitely fixes the case which was being complained about
and therefore is a step in the right direction.
It's a bit late here, so I'll push this in the morning and watch the
buildfarm.
Thanks!
Stephen
From cbca78a387ecfed9570bd079541ec7906c990f0f Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Thu, 3 Aug 2017 21:23:37 -0400
Subject: [PATCH] Fix ordering in pg_dump of GRANTs
The order in which GRANTs are output is important as GRANTs which have
been GRANT'd by individuals via WITH GRANT OPTION GRANTs have to come
after the GRANT which included the WITH GRANT OPTION. This happens
naturally in the backend during normal operation as we only change
existing ACLs in-place, only add new ACLs to the end, and when removing
an ACL we remove any which depend on it also.
Also, adjust the comments in acl.h to make this clear.
Unfortunately, the updates to pg_dump to handle initial privileges
involved pulling apart ACLs and then combining them back together and
could end up putting them back together in an invalid order, leading to
dumps which wouldn't restore.
Fix this by adjusting the queries used by pg_dump to ensure that the
ACLs are rebuilt in the same order in which they were originally.
Back-patch to 9.6 where the changes for initial privileges were done.
---
src/bin/pg_dump/dumputils.c | 51 -
src/include/utils/acl.h | 14 ++---
2 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index dfc6118..67049a6 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -722,21 +722,36 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
* We always perform this delta on all ACLs and expect that by the time
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
+ *
+ * Finally, the order in which privileges are in the ACL string (the order
+ * they been GRANT'd in, which the backend maintains) must be preserved to
+ * ensure that GRANTs WITH GRANT OPTION and subsequent GRANTs based on
+ * those are dumped in the correct order.
*/
- printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s as foo)",
+ printfPQExpBuffer(acl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS perm(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "AS init(init_acl) WHERE acl = init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
- printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s as foo)",
+ printfPQExpBuffer(racl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "AS permp(orig_acl) WHERE acl = orig_acl)) as foo)",
obj_kind,
acl_owner,
acl_column,
@@ -761,19 +776,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
- "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pip.initprivs) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s))) as foo) END",
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM
Re: [HACKERS] reload-through-the-top-parent switch the partition table
On 2017/08/04 1:08, David G. Johnston wrote: > On Thu, Aug 3, 2017 at 8:53 AM, Tom Lane wrote: > >> Robert Haas writes: >>> So maybe --load-via-partition-root if nobody likes my previous >>> suggestion of --partition-data-via-root ? >> >> WFM. >> > > +1 +1. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign table creation and NOT VALID check constraints
On 2017/08/04 2:13, Robert Haas wrote: > On Thu, Aug 3, 2017 at 12:35 AM, Tom Lane wrote: >> Robert Haas writes: >>> On Wed, Aug 2, 2017 at 9:41 PM, Amit Langote >>> wrote: Attached is a patch. I think this could be considered a bug-fix, backpatchable to 9.6 which introduced this behavior change [1]. >> >>> I could go either way on that. It's not inconceivable somebody could >>> be unhappy about seeing this behavior change in a minor release. >> >> FWIW, I vote with the camp that this is a clear bug and needs to be >> fixed. 9.6 broke a behavior that could be relied on before that. >> We do not normally hesitate to fix regressions in minor releases. >> >> (That's not a vote for the patch as submitted; I haven't reviewed it. >> But we need to fix this.) > > OK. I'm going to commit and back-patch the substantive fix with a > comment change, but I'm not going to include Amit's documentation > changes for now because I'm not sure they are going to be sufficiently > clear. There's not a lot of context for them where he put them. Thanks for committing the code changes. About the documentation changes, it seems that the only places where any description of NOT VALID appears is ALTER TABLE, ALTER FOREIGN TABLE, and ALTER DOMAIN references pages. Even if the CREATE (FOREIGN) TABLE syntax allows it, NOT VALID does not appear in the syntax synopsis, so it seems kind of a hidden feature. Maybe, we should fix that first (if at all). 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] map_partition_varattnos() and whole-row vars
On 2017/08/04 1:06, Robert Haas wrote: > On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita > wrote: >> On 2017/08/03 17:12, Amit Langote wrote: >>> Attached updated patches. >> >> Thanks for the patch! That looks good to me. > > Committed with some comment changes. Thanks a lot. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()
On 2017/08/04 3:29, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 1:04 AM, Amit Langote
> wrote:
>> Alright, attached updated 0001 does that.
>
> Committed 0001 and 0002.
Thanks.
> 0003 needs a rebase.
Rebased patch attached.
Thanks,
Amit
From f069845c027acc36aab4790d6d6afbf50bba803e Mon Sep 17 00:00:00 2001
From: amit
Date: Tue, 1 Aug 2017 10:58:38 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code
If the table being attached has attnos different from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to structural inequality of the corresponding Var expressions in the
the partition constraint and those in the table's check constraint
expressions. Fix this by changing the partition constraint's expressions
to bear the partition's attnos.
Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.
Reported by: Ashutosh Bapat
Report:
https://postgr.es/m/CAFjFpReT_kq_uwU_B8aWDxR7jNGE%3DP0iELycdq5oupi%3DxSQTOw%40mail.gmail.com
---
src/backend/commands/tablecmds.c | 40 ++-
src/test/regress/expected/alter_table.out | 45 +++
src/test/regress/sql/alter_table.sql | 38 ++
3 files changed, 111 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7859ef13ac..1b8d4b3d17 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13433,6 +13433,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
boolskip_validate = false;
ObjectAddress address;
const char *trigger_name;
+ boolfound_whole_row;
attachrel = heap_openrv(cmd->name, AccessExclusiveLock);
@@ -13614,6 +13615,16 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.
+*/
+ partConstraint = map_partition_varattnos(partConstraint, 1, attachrel,
+
rel, &found_whole_row);
+ /* There can never be a whole-row reference here */
+ if (found_whole_row)
+ elog(ERROR, "unexpected whole-row reference found in partition
key");
+
+ /*
* Check if we can do away with having to scan the table being attached
to
* validate the partition constraint, by *proving* that the existing
* constraints of the table *imply* the partition predicate. We include
@@ -13712,8 +13723,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
Relationpart_rel;
- Expr *constr;
- boolfound_whole_row;
+ List *my_partconstr = partConstraint;
/* Lock already taken */
if (part_relid != RelationGetRelid(attachrel))
@@ -13732,18 +13742,24 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
continue;
}
+ if (part_rel != attachrel)
+ {
+ /*
+* Adjust the constraint that we constructed
above for
+* attachRel so that it matches this
partition's attribute
+* numbers.
+*/
+ my_partconstr =
map_partition_varattnos(my_partconstr, 1,
+
part_rel, attachrel,
+
&found_whole_row);
+ /* There can never be a whole-row reference
here */
+ if (found_whole_row)
+ elog(ERROR, "unexpected whole-row
reference found in partition key");
+ }
+
/* Grab a work queue entry. */
tab = ATGetQueueEntry(wqueue, part_rel);
-
- /* Adjust constraint to match this partition */
- constr = linitial(partConst
Re: [HACKERS] analyzeCTE is too strict about typmods?
I wrote: > In short, therefore, it's looking to me like analyzeCTE() is wrong here. > It should allow the case where the recursive result has typmod -1 while > the non-recursive output column has some more-specific typmod, so long > as they match on type OID. That would correspond to what we do in > regular non-recursive UNION situations. Oh, scratch that. I was thinking that we could simply relax the error check, but that really doesn't work at all. The problem is that by now, we have probably already generated Vars referencing the outputs of the recursive CTE, and those will have the more-specific typmod, which is wrong in this scenario. Usually that wouldn't matter too much, but there are cases where it would matter. We could imagine dealing with this by re-parse-analyzing the recursive term using typmod -1 for the CTE output column, but I don't much want to go there. It wouldn't be cheap, and I'm not quite sure it's guaranteed to converge anyway. What's seeming like an attractive compromise is to change the HINT to recommend manually coercing the recursive term, instead of the non-recursive one. Adjusting the error cursor to point to that side might be a bit painful, but it's probably doable. Thoughts? 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] analyzeCTE is too strict about typmods?
I'm not sure why bug #7926 didn't get any love when filed, but the issue came up again today: https://www.postgresql.org/message-id/264036359.6712710.1501784552...@mail.yahoo.com and it does seem like this is pretty curious behavior. A minimal reproducer is regression=# create table base (f1 numeric(7,3)); CREATE TABLE regression=# with recursive foo as ( select f1 from base zunion all select f1+1 from foo ) select * from foo; ERROR: recursive query "foo" column 1 has type numeric(7,3) in non-recursive term but type numeric overall LINE 2: select f1 from base ^ HINT: Cast the output of the non-recursive term to the correct type. Now the thing about that is that the HINT's advice doesn't work: regression=# with recursive foo as ( select f1::numeric from base union all select f1+1 from foo ) select * from foo; ERROR: recursive query "foo" column 1 has type numeric(7,3) in non-recursive term but type numeric overall LINE 2: select f1::numeric from base ^ HINT: Cast the output of the non-recursive term to the correct type. The reason for this is that parse_coerce.c treats casting a value that's already of the required type to typmod -1 as a complete no-op (see first check in coerce_type_typmod). So the result is still just a Var for "f1". We could imagine fixing this by insisting that a RelabelType with typmod -1 should be plastered atop the expression in such cases. But I'm worried about the potential side-effects of that, and anyway I'm not convinced that parse_coerce.c is wrong to be doing it this way: typmod -1 generally means "unspecified typmod", so the bare Var seems like it ought to be considered to satisfy the typmod spec. Besdies, if you just do this: select f1 from base union all select f1+1 from base; it works, producing a UNION result deemed to have typmod -1, and there's no extra decoration added to the Var in the first leaf SELECT. In short, therefore, it's looking to me like analyzeCTE() is wrong here. It should allow the case where the recursive result has typmod -1 while the non-recursive output column has some more-specific typmod, so long as they match on type OID. That would correspond to what we do in regular non-recursive UNION situations. 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] Hash Functions
On Thu, Aug 3, 2017 at 6:08 PM, Andres Freund wrote:
>> That's another way to go, but it requires inventing a way to thread
>> the IV through the hash opclass interface.
>
> Only if we really want to do it really well :P. Using a hash_combine()
> like
>
> /*
> * Combine two hash values, resulting in another hash value, with decent bit
> * mixing.
> *
> * Similar to boost's hash_combine().
> */
> static inline uint32
> hash_combine(uint32 a, uint32 b)
> {
> a ^= b + 0x9e3779b9 + (a << 6) + (a >> 2);
> return a;
> }
>
> between hash(IV) and the hashfunction should do the trick (the IV needs
> to hashed once, otherwise the bit mix is bad).
That seems pretty lame, although it's sufficient to solve the
immediate problem, and I have to admit to a certain predilection for
things that solve the immediate problem without creating lots of
additional work.
>> We could:
>>
>> - Invent a new hash_partition AM that doesn't really make indexes but
>> supplies hash functions for hash partitioning.
>> - Add a new, optional support function 2 to the hash AM that takes a
>> value of the type *and* an IV as an argument.
>> - Something else.
>
> Not arguing for it, but one option could also have pg_type.hash*
> function(s).
True. That is a bit less configurable because you can't then have
multiple functions for the same type. Going through the opclass
interface means you can have hash_portable_ops and hash_fast_ops and
let people choose. But this would be easy to implement and enough for
most people in practice.
> One thing that I think might be advisable to think about is that we're
> atm stuck with a relatively bad hash function for hash indexes (and hash
> joins/aggs), and we should probably evolve it at some point. At the same
> time there's currently people out there relying on the current hash
> functions remaining stable.
That to me is a bit of a separate problem.
--
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] PL_stashcache, or, what's our minimum Perl version?
Andrew Dunstan writes:
> On 07/31/2017 06:54 PM, Tom Lane wrote:
>> but could we do something like
>> my $pflags = "PROVE_FLAGS='" . ($ENV{PROVE_FLAGS} || "--timer") . "'";
>> to allow overriding this choice from the buildfarm config?
> I have committed this in a slightly different form.
Thanks.
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] GSoC 2017: Foreign Key Arrays
To better understand a limitation I ask 5 questions What is the limitation? Why is there a limitation? Why is it a limitation? What can we do? Is it feasible? Through some reading: *What is the limitation?* presupposes that count(distinct y) has exactly the same notion of equality that the PK unique index has. In reality, count(distinct) will fall back to the default btree opclass for the array element type. the planner may choose an optimization of this sort when the index's opclass matches the one DISTINCT will use, ie the default for the data type. *Why is there a limitation?* necessary because ri_triggers.c relies on COUNT(DISTINCT x) on the element type, as well as on array_eq() on the array type, and we need those operations to have the same notion of equality that we're using otherwise. *Why is it a limitation?* That's wrong: DISTINCT should use the equality operator that corresponds to the index' operator class instead, not the default one. *What can we do ?* I'm sure that we can replace array_eq() with a newer polymorphic version but I don't know how we could get around COUNT(DISTINCT x) *Is it feasible? * I don't think I have the experience to answer that Best Regards, Mark Rofail
Re: [HACKERS] Hash Functions
On 2017-08-03 17:57:37 -0400, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 5:50 PM, Andres Freund wrote:
> > On 2017-08-03 17:43:44 -0400, Robert Haas wrote:
> >> For me, the basic point here is that we need a set of hash functions
> >> for hash partitioning that are different than what we use for hash
> >> indexes and hash joins -- otherwise when we hash partition a table and
> >> create hash indexes on each partition, those indexes will have nasty
> >> clustering. Partitionwise hash joins will have similar problems. So,
> >> a new set of hash functions specifically for hash partitioning is
> >> quite desirable.
> >
> > Couldn't that just as well solved by being a bit smarter with an IV? I
> > doubt we want to end up with different hashfunctions for sharding,
> > partitioning, hashjoins (which seems to form a hierarchy). Having a
> > working hash-combine function, or even better a hash API that can
> > continue to use the hash's internal state, seems a more scalable
> > solution.
>
> That's another way to go, but it requires inventing a way to thread
> the IV through the hash opclass interface.
Only if we really want to do it really well :P. Using a hash_combine()
like
/*
* Combine two hash values, resulting in another hash value, with decent bit
* mixing.
*
* Similar to boost's hash_combine().
*/
static inline uint32
hash_combine(uint32 a, uint32 b)
{
a ^= b + 0x9e3779b9 + (a << 6) + (a >> 2);
return a;
}
between hash(IV) and the hashfunction should do the trick (the IV needs
to hashed once, otherwise the bit mix is bad).
> That's actually sort of a
> problem anyway. Maybe I ought to have started with the question of
> how we're going to make that end of things work.
+1 one for that plan.
> We could:
>
> - Invent a new hash_partition AM that doesn't really make indexes but
> supplies hash functions for hash partitioning.
> - Add a new, optional support function 2 to the hash AM that takes a
> value of the type *and* an IV as an argument.
> - Something else.
Not arguing for it, but one option could also have pg_type.hash*
function(s).
One thing that I think might be advisable to think about is that we're
atm stuck with a relatively bad hash function for hash indexes (and hash
joins/aggs), and we should probably evolve it at some point. At the same
time there's currently people out there relying on the current hash
functions remaining stable.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hash Functions
On Thu, Aug 3, 2017 at 5:50 PM, Andres Freund wrote: > On 2017-08-03 17:43:44 -0400, Robert Haas wrote: >> For me, the basic point here is that we need a set of hash functions >> for hash partitioning that are different than what we use for hash >> indexes and hash joins -- otherwise when we hash partition a table and >> create hash indexes on each partition, those indexes will have nasty >> clustering. Partitionwise hash joins will have similar problems. So, >> a new set of hash functions specifically for hash partitioning is >> quite desirable. > > Couldn't that just as well solved by being a bit smarter with an IV? I > doubt we want to end up with different hashfunctions for sharding, > partitioning, hashjoins (which seems to form a hierarchy). Having a > working hash-combine function, or even better a hash API that can > continue to use the hash's internal state, seems a more scalable > solution. That's another way to go, but it requires inventing a way to thread the IV through the hash opclass interface. That's actually sort of a problem anyway. Maybe I ought to have started with the question of how we're going to make that end of things work. We could: - Invent a new hash_partition AM that doesn't really make indexes but supplies hash functions for hash partitioning. - Add a new, optional support function 2 to the hash AM that takes a value of the type *and* an IV as an argument. - Something else. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hash Functions
On Thu, Aug 3, 2017 at 5:32 PM, Andres Freund wrote: >> Do you have any feeling for which of those endianness-independent hash >> functions might be a reasonable choice for us? > > Not a strong / very informed one, TBH. > > I'm not convinced it's worth trying to achieve this in the first place, > now that we "nearly" have the insert-via-parent feature. Isn't this a > lot of work, for very little practical gain? Having to select that when > switching architectures still seems unproblematic. People will just > about never switch endianess, so even a tiny performance & effort > overhead doesn't seem worth it to me. I kind of agree with you. There are some advantages of being endian-independent, like maybe your hash partitioning is really across multiple shards, and not all the shards are the same machine architecture, but it's not going to come up for most people. For me, the basic point here is that we need a set of hash functions for hash partitioning that are different than what we use for hash indexes and hash joins -- otherwise when we hash partition a table and create hash indexes on each partition, those indexes will have nasty clustering. Partitionwise hash joins will have similar problems. So, a new set of hash functions specifically for hash partitioning is quite desirable. Given that, if it's not a big problem to pick ones that have the portability properties at least some people want, I'd be inclined to do it. If it results in a noticeable slowdown on macrobenchmarks, then not so much, but otherwise, I'd rather do what people are asking for than spend time arguing about it. -- 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] Add Roman numeral conversion to to_number
On Thu, Aug 3, 2017 at 2:54 PM, Oliver Ford wrote: > The formatting.c file specifies it as a TODO, so I thought implementing it > would be worthwhile. As there is a to_roman conversion having it the other > way is good for completeness. Huh, didn't know about that. Well, I'm not direly opposed to this or anything, just not sure that it's worth spending a lot of time on. > The existing int_to_roman code goes up to 3999 so this patch is consistent > with that. I could extend both to handle Unicode values for large numbers? Well, following what the existing code does is usually a good place to start -- whether you want to try to extend it is up to you. I'm not very interested in Roman numeral handling personally, so you might want to wait for some opinions from others. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hash Functions
Hi, On 2017-08-03 17:43:44 -0400, Robert Haas wrote: > For me, the basic point here is that we need a set of hash functions > for hash partitioning that are different than what we use for hash > indexes and hash joins -- otherwise when we hash partition a table and > create hash indexes on each partition, those indexes will have nasty > clustering. Partitionwise hash joins will have similar problems. So, > a new set of hash functions specifically for hash partitioning is > quite desirable. Couldn't that just as well solved by being a bit smarter with an IV? I doubt we want to end up with different hashfunctions for sharding, partitioning, hashjoins (which seems to form a hierarchy). Having a working hash-combine function, or even better a hash API that can continue to use the hash's internal state, seems a more scalable solution. Greetings, Andres Freund -- 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] Change in "policy" on dump ordering?
Michael Banck writes: > Am Donnerstag, den 27.07.2017, 15:52 -0400 schrieb Tom Lane: >> So I'm thinking we should consider the multi-pass patch I posted as >> a short-term, backpatchable workaround, which we could hope to >> replace with dependency logic later. > +1, it would be really nice if this could be fixed in the back-branches > before v11. Done. 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] Hash Functions
Hi, On 2017-08-03 17:09:41 -0400, Robert Haas wrote: > On Thu, Jun 1, 2017 at 2:25 PM, Andres Freund wrote: > > Just to clarify: I don't think it's a problem to do so for integers and > > most other simple scalar types. There's plenty hash algorithms that are > > endianess independent, and the rest is just a bit of care. > > Do you have any feeling for which of those endianness-independent hash > functions might be a reasonable choice for us? Not a strong / very informed one, TBH. I'm not convinced it's worth trying to achieve this in the first place, now that we "nearly" have the insert-via-parent feature. Isn't this a lot of work, for very little practical gain? Having to select that when switching architectures still seems unproblematic. People will just about never switch endianess, so even a tiny performance & effort overhead doesn't seem worth it to me. Leaving that aside: > https://github.com/markokr/pghashlib implements various hash functions > for PostgreSQL, and claims that, of those implemented, crc32, Jenkins, > lookup3be and lookup3le, md5, and siphash24 are endian-independent. > An interesting point here is that Jeff Davis asserted in the original > post on this thread that our existing hash_any() wasn't portable, but > our current hash_any seems to be the Jenkins algorithm -- so I'm > confused. Part of the problem seems to be that, according to > https://en.wikipedia.org/wiki/Jenkins_hash_function there are 4 of > those. I don't know whether the one in pghashlib is the same one > we've implemented. IIUC lookup3be/le from Marko's hashlib just has a endianess conversion added. I'd personally not go for jenkins3, it's not particularly fast, nor does it balance that out w/ being cryptographicaly secure. > Kennel Marshall suggested xxhash as an endian-independent algorithm > upthread. Code for that is available under a 2-clause BSD license. Yea, that'd have been one of my suggestions too. Especially as I still want to implement better compression using lz4, and that'll depend on xxhash in turn. > PostgreSQL page checksums use an algorithm based on, but not exactly, > FNV-1a. See storage/checksum_impl.h. The comments there say this > algorithm was chosen with speed in mind. Our version is not > endian-independent because it folds in 4-byte integers rather than > 1-byte integers, but plain old FNV-1a *is* endian-independent and > could be used. Non-SIMDed (which we hope to achieve with our implementation, which is why we have separate compiler flags for that file) implementations of FNV are, to my knowledge, not particularly fast. And the SIMD tricks are, to my knowledge, not really applicable to the case at hand here. So I'm not a fan of choosing FNV. > We also have an implementation of CRC32C in core - see port/pg_crc32.h > and port/pg_crc32c_sb8.c. It's not clear to me whether this is > Endian-independent or not, although there is stuff that depends on > WORDS_BIGENDIAN, so, uh, maybe? The results should be endian independent. It depends on WORDS_BIGENDIAN because we need different pre-computed tables depending on endianess. Greetings, Andres Freund -- 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] Support for Secure Transport SSL library on macOS as OpenSSL alternative
> On 03 Aug 2017, at 19:27, Michael Paquier wrote: > > On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson wrote: >> In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I >> presented a WIP patch for adding support for the Apple Secure Transport SSL >> library on macOS as, an alternative to OpenSSL. That patch got put on the >> backburner for a bit, but I’ve now found the time to make enough progress to >> warrant a new submission for discussions on this (and hopefully help >> hacking). >> >> It is a drop-in replacement for the OpenSSL code, and supports all the same >> features and options, except for two things: compression is not supported and >> the CRL cannot be loaded from a plain PEM file. A Keychain must be used for >> that instead. > > Is there a set of APIs to be able to get server certificate for the > frontend and the backend, and generate a hash of it? That matters for > channel binding support of SCRAM for tls-server-end-point. I believe we can use SSLCopyPeerTrust() for that. Admittedly I haven’t looked at that yet so need to get my head around channel binding, but it seems to fit the bill. > There were no APIs to get the TLS finish message last time I looked at OSX > stuff, which mattered for tls-unique. It would be nice if we could get one. Yeah, AFAICT there is no API for that. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hash Functions
On Thu, Jun 1, 2017 at 2:25 PM, Andres Freund wrote: > Just to clarify: I don't think it's a problem to do so for integers and > most other simple scalar types. There's plenty hash algorithms that are > endianess independent, and the rest is just a bit of care. Do you have any feeling for which of those endianness-independent hash functions might be a reasonable choice for us? https://github.com/markokr/pghashlib implements various hash functions for PostgreSQL, and claims that, of those implemented, crc32, Jenkins, lookup3be and lookup3le, md5, and siphash24 are endian-independent. An interesting point here is that Jeff Davis asserted in the original post on this thread that our existing hash_any() wasn't portable, but our current hash_any seems to be the Jenkins algorithm -- so I'm confused. Part of the problem seems to be that, according to https://en.wikipedia.org/wiki/Jenkins_hash_function there are 4 of those. I don't know whether the one in pghashlib is the same one we've implemented. Kennel Marshall suggested xxhash as an endian-independent algorithm upthread. Code for that is available under a 2-clause BSD license. PostgreSQL page checksums use an algorithm based on, but not exactly, FNV-1a. See storage/checksum_impl.h. The comments there say this algorithm was chosen with speed in mind. Our version is not endian-independent because it folds in 4-byte integers rather than 1-byte integers, but plain old FNV-1a *is* endian-independent and could be used. We also have an implementation of CRC32C in core - see port/pg_crc32.h and port/pg_crc32c_sb8.c. It's not clear to me whether this is Endian-independent or not, although there is stuff that depends on WORDS_BIGENDIAN, so, uh, maybe? Some other possibly-interesting links: https://research.neustar.biz/2011/12/29/choosing-a-good-hash-function-part-2/ http://greenrobot.org/essentials/features/performant-hash-functions-for-java/comparison-of-hash-functions/ https://www.strchr.com/hash_functions Thoughts? -- 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] PL_stashcache, or, what's our minimum Perl version?
On 07/31/2017 06:54 PM, Tom Lane wrote:
> but could we do something like
> my $pflags = "PROVE_FLAGS='" . ($ENV{PROVE_FLAGS} || "--timer") . "'";
>
> to allow overriding this choice from the buildfarm config?
>
>
I have committed this in a slightly different form.
cheers
andrew
--
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] xmltable SQL conformance
2017-08-03 22:54 GMT+02:00 Pavel Stehule : > > > 2017-08-03 22:14 GMT+02:00 Peter Eisentraut com>: > >> I'm looking to update the SQL conformance list for the release. I'm >> wondering whether the new xmltable feature fully completes the following >> items: >> >> X300XMLTable >> X301XMLTable: derived column list option >> X302XMLTable: ordinality column option >> X303XMLTable: column default option >> X304XMLTable: passing a context item >> >> It looks so to me, but maybe you know more. >> > > I am not sure what X304 means? Other are not supported 100% .. are related > to unsupported XQuery lang. > Others than x300, 301, 302, 303 are unsupported. I don't understand to X304 well - related syntax is ignored in Postgres if I understand. Pavel > > Regards > > Pavel > > > > >> >> -- >> Peter Eisentraut http://www.2ndQuadrant.com/ >> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >> > >
Re: [HACKERS] elog vs errmsg_internal
Peter Eisentraut writes: > Is there a preferred method to select between using elog() and > errmsg_internal()? ereport(... errmsg_internal() ...) can be a win for debug messages that are in hot code paths, because the test for whether the message will get printed is able to short-circuit more work. In particular, if you have moderately expensive functions like syscache lookups in the argument list of elog(), I believe those functions get evaluated even if we end up not printing anything. ereport() skips the arg-list evaluation in such cases. But if that doesn't seem very relevant, I'd tend to go for elog() just because it's less typing. 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] xmltable SQL conformance
2017-08-03 22:14 GMT+02:00 Peter Eisentraut < peter.eisentr...@2ndquadrant.com>: > I'm looking to update the SQL conformance list for the release. I'm > wondering whether the new xmltable feature fully completes the following > items: > > X300XMLTable > X301XMLTable: derived column list option > X302XMLTable: ordinality column option > X303XMLTable: column default option > X304XMLTable: passing a context item > > It looks so to me, but maybe you know more. > I am not sure what X304 means? Other are not supported 100% .. are related to unsupported XQuery lang. Regards Pavel > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] Macros bundling RELKIND_* conditions
Tom Lane wrote: > I think Peter's got the error and the detail backwards. It should be > more like > > ERROR: "someview" cannot have constraints > DETAIL: "someview" is a view. > > If we do it like that, we need one ERROR message per error reason, > and one DETAIL per relkind, which should be manageable. Hmm ... this works for me. Hopefully we'd have the "foo is a view" messages all centrally in pg_class.h (or maybe objectaddress, or some other central place). Then the "cannot have constraints" part would appear directly in whatever .c file is doing the check; no need to centralize in that case. -- Á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] elog vs errmsg_internal
Peter Eisentraut wrote: > Is there a preferred method to select between using elog() and > errmsg_internal()? > > Attached is a patch that converts some DEBUG messages to one of those > two to remove them from translation, but I'm not sure which one to pick > other than by random aesthetics. I think the contrib changes are probably not useful, since contrib is not a target for nls anyway. I'm not sure that all DEBUG messages should stay untranslated. If main log messages (i.e. NOTICE/LOG and above) are translated, why not debug? For example the ones in index.c and indexcmds.c. I agree that many of these debugs are useless when translated, but not all. Clearly, it's a judgement call which ones to mark for translation. Some other random thoughts in the same area: * I think translated messages in the server log are mostly an operational failure. I think we should default to C, perhaps offer translation as an option that's not enabled by default. Of course, messages sent to client should be translated just like currently. * Much worse is the fact that we send messages to the log in the database encoding. This means that having a server log mixing lines from databases with different encodings is a failure; it's just not possible to read the log at all. A simple fix would be to transliterate all messages to some common encoding (UTF8) so that at the log file can at least be opened. Another fix would be to have multiple log files, one per encoding; or maybe go for one per database. Neither of these proposals seem particularly great. Simply keeping the server log in C locale would fix this problem too. -- Á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
[HACKERS] elog vs errmsg_internal
Is there a preferred method to select between using elog() and
errmsg_internal()?
Attached is a patch that converts some DEBUG messages to one of those
two to remove them from translation, but I'm not sure which one to pick
other than by random aesthetics.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 52f9f20d4141f8537da1a012a7ab99207a92e016 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 2 May 2017 21:49:48 -0400
Subject: [PATCH] Remove DEBUG messages from translation
---
contrib/amcheck/verify_nbtree.c| 12 ++--
src/backend/access/transam/multixact.c | 10 +-
src/backend/access/transam/slru.c | 6 ++
src/backend/access/transam/varsup.c| 2 +-
src/backend/access/transam/xlog.c | 28 ++--
src/backend/catalog/dependency.c | 2 +-
src/backend/catalog/index.c| 5 ++---
src/backend/commands/indexcmds.c | 8
src/backend/commands/tablecmds.c | 6 +++---
src/backend/libpq/be-secure-openssl.c | 2 +-
src/backend/parser/parse_utilcmd.c | 2 +-
src/backend/postmaster/autovacuum.c| 9 +++--
src/backend/postmaster/bgworker.c | 13 +
src/backend/postmaster/checkpointer.c | 5 ++---
src/backend/postmaster/postmaster.c| 2 +-
src/backend/postmaster/syslogger.c | 3 +--
src/backend/replication/logical/launcher.c | 3 +--
src/backend/replication/logical/worker.c | 8
src/backend/replication/syncrep.c | 4 ++--
src/backend/replication/walsender.c| 4 ++--
src/backend/storage/lmgr/predicate.c | 2 +-
src/backend/storage/lmgr/proc.c| 4 ++--
src/backend/storage/smgr/md.c | 6 +++---
src/backend/tcop/postgres.c| 6 +++---
src/backend/utils/init/miscinit.c | 3 +--
25 files changed, 71 insertions(+), 84 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 9ae83dc839..ef5fc947ce 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -315,7 +315,7 @@ bt_check_every_level(Relation rel, bool readonly)
if (metad->btm_fastroot != metad->btm_root)
ereport(DEBUG1,
(errcode(ERRCODE_NO_DATA),
-errmsg("harmless fast root mismatch in index
%s",
+errmsg_internal("harmless fast root mismatch
in index %s",
RelationGetRelationName(rel)),
errdetail_internal("Fast root block %u (level
%u) differs from true root block %u (level %u).",
metad->btm_fastroot, metad->btm_fastlevel,
@@ -415,8 +415,8 @@ bt_check_level_from_leftmost(BtreeCheckState *state,
BtreeLevel level)
else
ereport(DEBUG1,
(errcode(ERRCODE_NO_DATA),
-errmsg("block %u of index
\"%s\" ignored",
- current,
RelationGetRelationName(state->rel;
+errmsg_internal("block %u of
index \"%s\" ignored",
+
current, RelationGetRelationName(state->rel;
goto nextpage;
}
else if (nextleveldown.leftmost == InvalidBlockNumber)
@@ -823,8 +823,8 @@ bt_right_page_check_scankey(BtreeCheckState *state)
targetnext = opaque->btpo_next;
ereport(DEBUG1,
(errcode(ERRCODE_NO_DATA),
-errmsg("level %u leftmost page of index \"%s\"
was found deleted or half dead",
- opaque->btpo.level,
RelationGetRelationName(state->rel)),
+errmsg_internal("level %u leftmost page of
index \"%s\" was found deleted or half dead",
+
opaque->btpo.level, RelationGetRelationName(state->rel)),
errdetail_internal("Deleted page found when
building scankey from right sibling.")));
/* Be slightly more pro-active in freeing this memory, just in
case */
@@ -950,7 +950,7 @@ bt_right_page_check_scankey(BtreeCheckState *state)
*/
ereport(DEBUG1,
(errcode(ERRCODE_NO_DATA),
-errmsg("%s block %u of index \"%s\" has no
first data item",
+errmsg_internal("%s block %u of index
[HACKERS] xmltable SQL conformance
I'm looking to update the SQL conformance list for the release. I'm wondering whether the new xmltable feature fully completes the following items: X300XMLTable X301XMLTable: derived column list option X302XMLTable: ordinality column option X303XMLTable: column default option X304XMLTable: passing a context item It looks so to me, but maybe you know more. -- 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] Add Roman numeral conversion to to_number
On Thursday, 3 August 2017, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford > wrote:
> > Adds to the to_number() function the ability to convert Roman numerals
> > to a number. This feature is on the formatting.c TODO list. It is not
> > currently implemented in either Oracle, MSSQL or MySQL so gives
> > PostgreSQL an edge :-)
>
> I kind of put my head in my hands when I saw this. I'm not really
> sure it's worth complicating the code for something that has so little
> practical utility, but maybe other people will feel differently. I
> can't deny the profound advantages associated with having a leg up on
> Oracle.
The formatting.c file specifies it as a TODO, so I thought implementing it
would be worthwhile. As there is a to_roman conversion having it the other
way is good for completeness.
>
> The error reporting is a little wonky, although maybe no wonkier than
> anything else about these conversion routines.
>
> rhaas=# select to_number('q', 'rn');
> ERROR: invalid character "q"
>
> (hmm, no position)
>
> rhaas=# select to_number('dd', 'rn');
> ERROR: invalid character "D" at position 1
>
> (now i get a position, but it's not really the right position; and the
> problem isn't really that the character is invalid but that you don't
> like me including it twice, and I said 'd' not 'D')
>
> rhaas=# select to_number('à', 'rn');
> ERROR: invalid character "?"
>
> (eh?)
>
> How much call is there for a format that can only represent values up to
> 3999?
>
>
The existing int_to_roman code goes up to 3999 so this patch is consistent
with that. I could extend both to handle Unicode values for large numbers?
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
Re: [HACKERS] Add Roman numeral conversion to to_number
On Thu, Aug 03, 2017 at 01:45:02PM -0400, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford wrote:
> > Adds to the to_number() function the ability to convert Roman numerals
> > to a number. This feature is on the formatting.c TODO list. It is not
> > currently implemented in either Oracle, MSSQL or MySQL so gives
> > PostgreSQL an edge :-)
>
> I kind of put my head in my hands when I saw this. I'm not really
> sure it's worth complicating the code for something that has so little
> practical utility, but maybe other people will feel differently. I
> can't deny the profound advantages associated with having a leg up on
> Oracle.
>
> The error reporting is a little wonky, although maybe no wonkier than
> anything else about these conversion routines.
>
> rhaas=# select to_number('q', 'rn');
> ERROR: invalid character "q"
>
> (hmm, no position)
>
> rhaas=# select to_number('dd', 'rn');
> ERROR: invalid character "D" at position 1
>
> (now i get a position, but it's not really the right position; and the
> problem isn't really that the character is invalid but that you don't
> like me including it twice, and I said 'd' not 'D')
>
> rhaas=# select to_number('à', 'rn');
> ERROR: invalid character "?"
>
> (eh?)
>
> How much call is there for a format that can only represent values up to 3999?
There are ways to represent much larger numbers, possibly bigger than
INT_MAX. https://en.wikipedia.org/wiki/Roman_numerals#Large_numbers
https://en.wikipedia.org/wiki/Numerals_in_Unicode#Roman_numerals
As nearly as I can tell, this patch is late by 124 days.
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()
On Thu, Aug 3, 2017 at 1:04 AM, Amit Langote wrote: > Alright, attached updated 0001 does that. Committed 0001 and 0002. 0003 needs a rebase. -- 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] pgbench: Skipping the creating primary keys after initialization
For the CREATE stuff, the script language is SQL, the command to use it is "psql"... The real and hard part is to fill tables with meaningful pseudo-random test data which do not violate constraints for any non trivial schema involving foreign keys and various unique constraints. The solution for this is SQL for trivial cases, think of: "INSERT INTO Foo() SELECT ... FROM generate_series(...);" Yeah. I was also thinking that complicated data-generation requirements could be handled with plpgsql DO blocks, avoiding the need for hard-wired code inside pgbench. I do not thing that it is really be needed for what pgbench does, though. See attached attempt, including a no_foreign_keys option. The only tricky thing is to have the elapsed/remaining advancement report on stdout, maybe with some PL/pgSQL. Timings are very similar compared to "pgbench -i". -- Fabien. pgbench_init.sql Description: application/sql -- 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] Tightly packing expressions
Hi Doug, On 2017-08-03 18:01:06 +, Douglas Doole wrote: > Back when you were getting ready to commit your faster expressions, I > volunteered to look at switching from an array of fixed sized steps to > tightly packed structures. ( > https://www.postgresql.org/message-id/20170314221648.jrcgh5n7ld4ej...@alap3.anarazel.de). > I've finally found time to make it happen. Thanks for working on it! > Using tightly packed structures makes the expressions a lot smaller. I > instrumented some before and after builds and ran "make check" just to see > how much memory expressions were using. What I found was: > > There were ~104K expressions. > > Memory - bytes needed to hold the steps of the expressions > Allocated Memory - memory is allocated in blocks, this is the bytes > allocated to hold the expressions > Wasted Memory - the difference between the allocated and used memory > > Original code: > Memory: min=64 max=9984 total=28232512 average=265 > Allocated Memory: min=1024 max=16384 total=71584 average=1045 > Wasted Memory: min=0 max=6400 total=82939072 average=780 > > New code: > Memory: min=32 (50%) max=8128 (82%) total=18266712 (65%) average=175 (66%) > Allocated Memory: min=192 (19%) max=9408 (57%) total=29386176 (26%) > average=282 (27%) > Wasted Memory: min=0 (0%) max=1280 (20%) total=9464 (13%) average=106 > (14%) That's actually not *that* big of a saving... > Another benefit of the way I did this is that the expression memory is > never reallocated. This means that it is safe for one step to point > directly to a field in another step without needing to separately palloc > storage. That should allow us to simplify the code and reduce pointer > traversals. (I haven't exploited this in the patch. I figured it would be a > task for round 2 assuming you like what I've done here.) Yes, that's a neat benefit. Although I think it'd even better if we could work to the point where the mutable and the unchanging data is separately allocated, so we can at some point avoid redundant expression "compilations". > The work was mostly mechanical. The only tricky bit was dealing with the > places where you jump to step n+1 while building step n. Since we can't > tell the address of step n+1 until it is actually built, I had to defer > resolution of the jumps. So all the interesting bits of this patch are in > ExprEvalPushStep(). I was wondering about a more general "symbol resolution" stage anyway. Then we'd allocate individual steps during ExecInitExprRec, and allocate the linear array after we know the exact size. I think it'd be important to see some performance measurements, especially for larger queries. It'd not be too surprising if the different method of dereffing the next expression actually has a negative performance effect, but I'm absolutely not sure of that. Regards, 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] Unused variable scanned_tuples in LVRelStats
On Thu, Aug 3, 2017 at 1:10 AM, Masahiko Sawada wrote: > So we can remove scanned_tuples from LVRelStats struct and change the > variable name num_tuples to scanned_tuples. Attached updated patch. On second thought, I think we should just leave this alone. scanned_tuples is closely tied to tupcount_pages, so it's a little confusing to pull one out and not the other. And we can't pull tupcount_pages out of LVRelStats because lazy_cleanup_index needs it. The current situation isn't doing any harm, so I'm not seeing much point in changing it. -- 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] Add Roman numeral conversion to to_number
On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford wrote:
> Adds to the to_number() function the ability to convert Roman numerals
> to a number. This feature is on the formatting.c TODO list. It is not
> currently implemented in either Oracle, MSSQL or MySQL so gives
> PostgreSQL an edge :-)
I kind of put my head in my hands when I saw this. I'm not really
sure it's worth complicating the code for something that has so little
practical utility, but maybe other people will feel differently. I
can't deny the profound advantages associated with having a leg up on
Oracle.
The error reporting is a little wonky, although maybe no wonkier than
anything else about these conversion routines.
rhaas=# select to_number('q', 'rn');
ERROR: invalid character "q"
(hmm, no position)
rhaas=# select to_number('dd', 'rn');
ERROR: invalid character "D" at position 1
(now i get a position, but it's not really the right position; and the
problem isn't really that the character is invalid but that you don't
like me including it twice, and I said 'd' not 'D')
rhaas=# select to_number('à', 'rn');
ERROR: invalid character "?"
(eh?)
How much call is there for a format that can only represent values up to 3999?
--
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] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson wrote: > In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I > presented a WIP patch for adding support for the Apple Secure Transport SSL > library on macOS as, an alternative to OpenSSL. That patch got put on the > backburner for a bit, but I’ve now found the time to make enough progress to > warrant a new submission for discussions on this (and hopefully help hacking). > > It is a drop-in replacement for the OpenSSL code, and supports all the same > features and options, except for two things: compression is not supported and > the CRL cannot be loaded from a plain PEM file. A Keychain must be used for > that instead. Is there a set of APIs to be able to get server certificate for the frontend and the backend, and generate a hash of it? That matters for channel binding support of SCRAM for tls-server-end-point. There were no APIs to get the TLS finish message last time I looked at OSX stuff, which mattered for tls-unique. It would be nice if we could get one. -- 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] Cache lookup errors with functions manipulation object addresses
On Fri, Jul 21, 2017 at 8:53 AM, Michael Paquier wrote: > I can see your point. The v1 proposed above adds quite a lot of error > code churn to deal with the lack of missing_ok flag in > getObjectDescription, getObjectIdentity and getObjectIdentityParts. > And the cache lookup error messages cannot be object-specific either, > so I fell back to using %u/%u with the class as first identifier. > Let's go with what you suggest here then. Thinking more on that, I'll go with the flag Alvaro suggested. > Before producing any v2, I would still need some sort of consensus > about a couple of points though to grab object details. Here is what I > think should be done: > 1) For functions, let's remove format_procedure_qualified, and replace > it with format_procedure_extended which replaces > format_procedure_internal. format_procedure_qualified is only used for objaddr.c, so I am going here for not defining a compatibility set of macros. > 2) For relation attributes, we have now get_relid_attribute_name() > which cannot tolerate failures, and get_attname which returns NULL on > errors. My suggestion here is to remove get_relid_attribute_name, and > add a missing_ok flag to get_attname. Let's do this as a refactoring > patch. One thing that may matter is modules calling the existing APIs. > We could provide a compatibility macro. But here get_relid_attribute_name is old enough to have a compatibility macro. So I'll add one in one of the refactoring patches. > 3) For types, the existing interface is more a mess. HEAD has > allow_invalid which is used by the SQL function format_type. My > suggestion here would be to remove allow_invalid and replace it with > missing_ok, to return NULL if the type has gone missing, or issue an > error depending on what caller wants. oidvectortypes() needs to be > modified as well. I would suggest to change this interface as a > refactoring patch. With compatibility macros. > 4) GetForeignServer and GetForeignDataWrapper need to have a > missing_ok. I suggest to refactor them as well with a separate patch. > 5) For operators, there is format_operator_internal which has its own > idea of invalid objects. I think that the existing API should be > reworked. No convinced much here, format_operator_qualified is not widely used as far as I see, so I would just replace it with the _extended version. > So, while the work of this thread is largely possible and can be done > incrementally. The devil is in the details of how to handle the > existing APIs. Reaching an agreement about all the points above is key > to get a clean implementation we are happy with. Extra opinions always welcome. -- 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] Subscription code improvements
On 7/13/17 23:53, Masahiko Sawada wrote: > To summary, I think we now have only one issue; ALTER SUBSCRIPTION is > not transactional, 0004 patch is addressing this issue . We have two competing patches for this issue. This patch moves the killing to the end of the DDL transaction. Your earlier patch makes the tablesync work itself responsible for exiting. Do you wish to comment which direction to pursue? (Doing both might also be an option?) -- 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] foreign table creation and NOT VALID check constraints
On Thu, Aug 3, 2017 at 12:35 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Aug 2, 2017 at 9:41 PM, Amit Langote >> wrote: >>> Attached is a patch. I think this could be considered a bug-fix, >>> backpatchable to 9.6 which introduced this behavior change [1]. > >> I could go either way on that. It's not inconceivable somebody could >> be unhappy about seeing this behavior change in a minor release. > > FWIW, I vote with the camp that this is a clear bug and needs to be > fixed. 9.6 broke a behavior that could be relied on before that. > We do not normally hesitate to fix regressions in minor releases. > > (That's not a vote for the patch as submitted; I haven't reviewed it. > But we need to fix this.) OK. I'm going to commit and back-patch the substantive fix with a comment change, but I'm not going to include Amit's documentation changes for now because I'm not sure they are going to be sufficiently clear. There's not a lot of context for them where he put them. -- 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 comments in nodeModifyTable.c
On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita wrote: > I updated the patch that way. Attached is a new version of the patch. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reload-through-the-top-parent switch the partition table
On Thu, Aug 3, 2017 at 8:53 AM, Tom Lane wrote: > Robert Haas writes: > > So maybe --load-via-partition-root if nobody likes my previous > > suggestion of --partition-data-via-root ? > > WFM. > +1 David J.
Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables
On Thu, Aug 3, 2017 at 2:10 AM, Robert Haas wrote:
> On Mon, Jul 31, 2017 at 7:59 AM, Ashutosh Bapat
> wrote:
>> Adding AppendRelInfos to root->append_rel_list as and when they are
>> created would keep parent AppendRelInfos before those of children. But
>> that function throws away the AppendRelInfo it created when their are
>> no real children i.e. in partitioned table's case when has no leaf
>> partitions. So, we can't do that. Hence, I chose to change the API to
>> return the list of AppendRelInfos when the given RTE has real
>> children.
>
> So, IIUC, the case you're concerned about is when you have a hierarchy
> of only partitioned tables, with no plain tables. For example, if B
> is a partitioned table and a partition of A, and that's all there is,
> A will recurse to B and B will return NIL.
>
> Is it necessary to get rid of the extra AppendRelInfos, or are they
> harmless like the duplicate RTE and PlanRowMark nodes?
>
Actually there are two sides to this:
If there are no leaf partitions, without the patch two things happen
1. rte->inh is cleared and 2 no appinfo is added to the
root->append_rel_list, even though harmless RTE and PlanRowMark nodes
are created. The first avoids treating the relation as the inheritance
parent and thus avoids creating any child relations and paths, saving
a lot of work. Ultimately set_rel_size() marks such a relation as
dummy
352 else if (rte->relkind == RELKIND_PARTITIONED_TABLE)
353 {
354 /*
355 * A partitioned table without leaf
partitions is marked
356 * as a dummy rel.
357 */
358 set_dummy_rel_pathlist(rel);
359 }
Since root->append_rel_list is traversed for every inheritance parent,
not adding needless AppendRelInfos improves performance and saves
memory, (FWIW or consider a case where there are thousands of
partitioned partitions without any leaf partition.).
My initial thought was to keep both these properties intact. But then
removing such AppendRelInfos would have a problem when such a table is
on the inner side of the join as described in [1]. So I wrote the
patch not to do either of those things when there are partitioned
partitions without leaf partitions. So, it looks like you are correct,
we could just go ahead and add those AppendRelInfos directly to
root->append_rel_list.
> /*
> * If all the children were temp tables or a partitioned parent did not
> * have any leaf partitions, pretend it's a non-inheritance situation; we
> * don't need Append node in that case. The duplicate RTE we added for
> * the parent table is harmless, so we don't bother to get rid of it;
> * ditto for the useless PlanRowMark node.
> */
> if (!need_append)
> {
> /* Clear flag before returning */
> rte->inh = false;
> return;
> }
>
> If we do need to get rid of the extra AppendRelInfos, maybe a less
> invasive solution would be to change the if (!need_append) case to do
> root->append_rel_list = list_truncate(root->append_rel_list,
> original_append_rel_length).
We might require this for non-partitioned tables. I will try to
implement it this way in the next set of patches.
>
>> The code avoids creating AppendRelInfos for a child which represents
>> the parent in its role as a simple member of inheritance set.
>
> OK, I suggest we rewrite the whole comment like this: "We need an
> AppendRelInfo if paths will be built for the child RTE. If
> childrte->inh is true, then we'll always need to generate append paths
> for it. If childrte->inh is false, we must scan it if it's not a
> partitioned table; but if it is a partitioned table, then it never has
> any data of its own and need not be scanned. It does, however, need
> to be locked, so note the OID for inclusion in the
> PartitionedChildRelInfo we're going to build."
Done.
>
> It looks like you also need to update the header comment for
> AppendRelInfo itself, in nodes/relation.h.
Done. Thanks for pointing it out.
>
> + * PlannerInfo for every child is obtained by translating relevant
> members
>
> Insert "The" at the start of the sentence.
Done.
>
> -subroot->parse = (Query *)
> -adjust_appendrel_attrs(root,
> - (Node *) parse,
> - appinfo);
> +subroot->parse = (Query *) adjust_appendrel_attrs(parent_root,
> + (Node *)
> parent_parse,
> + 1, &appinfo);
>
> I suggest that you don't remove the line break after the cast.
This is part of 0001 patch, fixed there.
>
> + * If the child is further partitioned, remember it as a parent.
> Since
> + * partitioned tables do not have any data, we don't need to create
> + * plan for it. We just need its Planne
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita wrote: > On 2017/08/03 17:12, Amit Langote wrote: >> Attached updated patches. > > Thanks for the patch! That looks good to me. Committed with some comment changes. -- 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
[HACKERS] Reminder: back-branch releases next week
[ Shoulda got this out sooner, but better late than never ] We'll be doing routine quarterly releases of supported Postgres back branches next week (tarballs wrapped Monday 7th, public announcement Thursday 10th). We'll release v10 beta3 at the same time. 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] reload-through-the-top-parent switch the partition table
Robert Haas writes: > So maybe --load-via-partition-root if nobody likes my previous > suggestion of --partition-data-via-root ? WFM. 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] On Complex Source Code Reading Strategy
On Thu, Aug 3, 2017 at 1:55 AM, Zeray Kalayu wrote: > Therefore, I feel and think that I am a bit in a hurry to be DB > hacker. But given time, indefatigability and PG community support, I > believe that I will manage to achieve my dream. Indefatigability is key. -- 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] Macros bundling RELKIND_* conditions
Joe Conway wrote: > On 08/02/2017 10:52 PM, Ashutosh Bapat wrote: > > On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera > > wrote: > >> Alvaro Herrera wrote: > >>> I think pg_class is a reasonable place to put more generic relkind lists > >>> alongside a matching error message for each, rather than specialized > >>> "does this relkind have storage" macros. What about something like a > >>> struct list in pg_class.h, > >> > >> I just noticed that this doesn't help at all with the initial problem > >> statement, which is that some of the relkind checks failed to notice > >> that partitioned tables needed to be added to the set. Maybe it still > >> helps because you have something to grep for, as Tom proposed elsewhere. > > > > Having something like relkind_i_t_T, though correct, doesn't make the > > test readable. That's another improvement because of using such > > macros. The macro name tells the purpose of the test, which is what a > > reader would be interested in, rather than the actual values used. > > +1 So add another layer: #define RELKIND_HAS_STORAGE relkind_i_t_T -- Á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] intermittent failures in Cygwin from select_parallel tests
On Wed, Aug 2, 2017 at 11:47 PM, Noah Misch wrote:
> postmaster algorithms rely on the PG_SETMASK() calls preventing that. Without
> such protection, duplicate bgworkers are an understandable result. I caught
> several other assertions; the PMChildFlags failure is another case of
> duplicate postmaster children:
>
> 6 TRAP: FailedAssertion("!(entry->trans == ((void *)0))", File:
> "pgstat.c", Line: 871)
> 3 TRAP: FailedAssertion("!(PMSignalState->PMChildFlags[slot] == 1)",
> File: "pmsignal.c", Line: 229)
> 20 TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c",
> Line: 2523)
> 21 TRAP: FailedAssertion("!(vmq->mq_sender == ((void *)0))", File:
> "shm_mq.c", Line: 221)
> Also, got a few "select() failed in postmaster: Bad address"
>
> I suspect a Cygwin signals bug. I'll try to distill a self-contained test
> case for the Cygwin hackers. The lack of failures on buildfarm member brolga
> argues that older Cygwin is not affected.
Nice detective work.
--
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] reload-through-the-top-parent switch the partition table
On Wed, Aug 2, 2017 at 4:08 PM, Peter Eisentraut wrote: > On 8/2/17 13:58, Tom Lane wrote: >> I notice that the option list already includes some references to >> "insert", so maybe "--insert-via-partition-root"? Although you could >> argue that that's confusing when we're using COPY. > > "load" could be more general. But I'm also OK with "restore". "load" seems better than "restore" to me, both because it's shorter and because it sounds less like pg_dump will be doing the job of pg_restore. So maybe --load-via-partition-root if nobody likes my previous suggestion of --partition-data-via-root ? -- 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] reload-through-the-top-parent switch the partition table
On Thu, Aug 3, 2017 at 9:01 AM, Rushabh Lathia wrote: > --unpartition-partitioned-table is very confusing. +1. > I liked the previous option which is --use-partitioned-table > [partition-name, ...]. > The Only problem with --use-partitioned-table is, a user needs to specify > the > partition-name in the options list. Imagine if someone having 100's of > partitions then specifying those name is pg_dump option is a pain. Yeah, that won't work. > Rather than that: > > --use-partitioned-table [partitioned_name, ...] # if > names are omitted it defaults to all the partitioned tables. > > Here user need to specify the root relation name in the option - and any > partition table have that as a ROOT, will load the data through > top-parent-relation. We could do that, but I'm not sure it's a good idea to use getopt_long() with optional options. Sometimes that creates confusion -- is pg_dump --use-partitioned-table salad an attempt to dump the salad database with the --use-partitioned-table option, or an attempt to apply --use-partitioned-table only to partitions whose parent is the salad table? getopt_long() has an answer, but some people may guess incorrectly about what it is. I would be more inclined to make this a global option than something that modifies the behavior for certain tables; the only per-table flags we have right now are just to include/exclude individual tables. You could make --inserts or --no-unlogged-table-data apply to some but not all tables, but we didn't; why start here? I don't like the specific name --use-partitioned-table much either. Use it for what? -- 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] pgbench: Skipping the creating primary keys after initialization
Masahiko Sawada writes:
> If we want to create other tables and load data to them as we want we
> can do that using psql -f. So an alternative ways is having a flexible
> style option for example --custom-initialize = { [load, create_pkey,
> create_fkey, vacuum], ... }. That would solve this in a better way.
FWIW, I like that significantly better than your original proposal.
It'd allow people to execute parts of pgbench's standard initialization
sequence and then do other things in between (in psql). Realistically,
that's probably about as much win as we need here --- if you're veering
far enough away from the standard scenario that that doesn't do it for
you, you might as well just write an all-custom setup script in psql.
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] Macros bundling RELKIND_* conditions
On 08/02/2017 10:52 PM, Ashutosh Bapat wrote: > On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera > wrote: >> Alvaro Herrera wrote: >>> I think pg_class is a reasonable place to put more generic relkind lists >>> alongside a matching error message for each, rather than specialized >>> "does this relkind have storage" macros. What about something like a >>> struct list in pg_class.h, >> >> I just noticed that this doesn't help at all with the initial problem >> statement, which is that some of the relkind checks failed to notice >> that partitioned tables needed to be added to the set. Maybe it still >> helps because you have something to grep for, as Tom proposed elsewhere. > > Having something like relkind_i_t_T, though correct, doesn't make the > test readable. That's another improvement because of using such > macros. The macro name tells the purpose of the test, which is what a > reader would be interested in, rather than the actual values used. +1 Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Macros bundling RELKIND_* conditions
On 08/02/2017 10:30 PM, Ashutosh Bapat wrote: > On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas wrote: >> 0001-RELKIND_HAS_VISIBILITY_MAP.patch - one place >> 0002-RELKIND_HAS_STORAGE.patch - one place >> 0003-RELKIND_HAS_XIDS-macro.patch - one place >> 0004-RELKIND_HAS_COMPOSITE_TYPE-macro.patch - one place >> 0005-RELKIND_CAN_HAVE_TOAST_TABLE-macro.patch - one place >> 0006-RELKIND_CAN_HAVE_COLUMN_COMMENT-macro.patch - one place >> 0007-RELKIND_CAN_HAVE_INDEX-macro.patch - two places >> 0008-RELKIND_CAN_HAVE_COLUMN_SECLABEL-macro.patch - one place >> 0009-RELKIND_CAN_HAVE_STATS-macro.patch - two places >> >> I'm totally cool with doing this where we can use the macro in more >> than one place, but otherwise I don't think it helps. I disagree. > Can we say that any relation that has visibility map will also have > xids? If yes, what's that common property? Perhaps there are better ways to achieve the same goal (e.g. nearby comments), but I think this is the salient point. The macro names allow whoever is looking at the code to focus on the relevant properties of the relkind without having to arrive at the conclusion themselves that relkind "A" has property "B". Makes the code easier to read and understand IMHO. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Fabien COELHO writes: > As for a more generic solution, the easy part are the "CREATE" stuff and > the transaction script stuff (existing pgbench scripts). > For the CREATE stuff, the script language is SQL, the command to use it is > "psql"... > The real and hard part is to fill tables with meaningful pseudo-random > test data which do not violate constraints for any non trivial schema > involving foreign keys and various unique constraints. > The solution for this is SQL for trivial cases, think of: >"INSERT INTO Foo() SELECT ... FROM generate_series(...);" Yeah. I was also thinking that complicated data-generation requirements could be handled with plpgsql DO blocks, avoiding the need for hard-wired code inside pgbench. 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] Row Level Security Documentation
On Thu, Jul 13, 2017 at 5:49 AM, Fabien COELHO wrote: > > Hello Rod, > > This version of the table attempts to stipulate which section of the >> process the rule applies to. >> > > The table should be referenced from the description, something like "Table > xxx summarizes the ..." > Added the below which seemed consistent with other "see something else" messages. A summary of the application of policies to a command is found in . > ISTM that it would be clearer to split the Policy column into "FOR xxx > ..." and "USING" or "WITH CHECK", and to merge the rows which have the same > "FOR xxx ..." contents, something like: > >POLICY | > ---++- > | USING | ... > FOR ALL ...++- > | WITH CHECK | ... > ---++- > FOR SELECT ... | USING | ... > > So that it is clear that only ALL & UPDATE can get both USING & WITH > CHECK. This can be done with "morerows=1" on an entry so that it spans more > rows. > Done. I couldn't figure out a morecols=1 equivalent to keep everything under the Policy heading without a full colspec. > For empty cells, maybe a dash would be clearer. Not sure. Looked cluttered to me. Tried N/A first which was even worse. -- Rod Taylor diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml index c0dfe1ea4b..52a868e65d 100644 --- a/doc/src/sgml/ref/create_policy.sgml +++ b/doc/src/sgml/ref/create_policy.sgml @@ -94,6 +94,11 @@ CREATE POLICY name ON default deny policy is assumed, so that no rows will be visible or updatable. + + + A summary of the application of policies to a command is found + in . + @@ -389,6 +394,80 @@ CREATE POLICY name ON + + Policies Applied During Statement + + + + + + + + + + Policy + SELECT + INSERT + UPDATE + DELETE + + + + + FOR ALL ... + USING + WHERE clause + RETURNING clause + WHERE and RETURNING clause + WHERE and RETURNING clause + + + WITH CHECK + + new tuple + new tuple + new tuple + + + FOR SELECT ... USING + WHERE clause + RETURNING clause + WHERE and RETURNING clause + WHERE and RETURNING clause + + + FOR INSERT ... WITH CHECK + + new tuple + + + + + FOR UPDATE ... + USING + + + WHERE clause + + + + WITH CHECK + + + new tuple + + + + FOR DELETE ... USING + + + + WHERE clause + + + + + -- 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] reload-through-the-top-parent switch the partition table
On Thursday, August 3, 2017, Rushabh Lathia wrote: > > > --use-partitioned-table [partitioned_name, ...] # if > names are omitted it defaults to all the partitioned tables. > > Here user need to specify the root relation name in the option - and any > partition table have that as a ROOT, will load the data through > top-parent-relation. > > This is indeed what I intended to convey. Only specify "root" names. David J.
Re: [HACKERS] Unused field of ProjectionPath
Antonin Houska writes: > I've noticed that the dummypp field of ProjectionPath is set but never read. I do not think removing that field is a good idea, even if it's not used in the current form of the logic. It's useful for debugging purposes at least. See original commit comment at https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=8b9d323cb9810109e3e5aab1ead427cbbb7aa77e 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] pgsql 10: hash indexes testing
On Wed, Aug 02, 2017 at 11:34:13AM -0400, Robert Haas wrote: > On Wed, Jul 12, 2017 at 1:10 AM, Amit Kapila wrote: > > It seems so. Basically, in the case of a large number of duplicates, > > we hit the maximum number of overflow pages. There is a theoretical > > possibility of hitting it but it could also happen that we are not > > free the existing unused overflow pages due to which it keeps on > > growing and hit the limit. I have requested up thread to verify if > > that is happening in this case and I am still waiting for same. The > > squeeze operation does free such unused overflow pages after cleaning > > them. As this is a costly operation and needs a cleanup lock, so we > > currently perform it only during Vacuum and next split from the bucket > > which can have redundant overflow pages. > > Oops. It was rather short-sighted of us not to increase > HASH_MAX_BITMAPS when we bumped HASH_VERSION. Actually removing that > limit is hard, but we could have easily bumped it for 128 to say 1024 > without (I think) causing any problem, which would have given us quite > a bit of headroom here. I suppose we could still try to jam that > change in before beta3 (bumping HASH_VERSION again) but that might be > asking for trouble. I, for one, would be grateful for such a bump (or better). Currently having more fun than I wish trying to figure out where my inserts will begin to fail so that I don't make a mess of things. Right now I can't match data going in with sane partitioning points in-storage. If I can go "3 months worth of data then partition" or the like and have enough room for differences in the data then I can be pretty happy. ATM I'm not getting anywhere near that and am tempted to chuck it all in, eat the 3-4x disk space cost and go back to btree which'd cost me terrabytes. AP -- 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] reload-through-the-top-parent switch the partition table
On Thu, Aug 3, 2017 at 3:39 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Aug 2, 2017 at 11:47 PM, David G. Johnston > wrote: > > On Wed, Aug 2, 2017 at 10:58 AM, Tom Lane wrote: > >> > >> Robert Haas writes: > >> > On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane wrote: > >> >> --restore-via-partition-root ? > >> > >> > I worry someone will think that pg_dump is now restoring stuff, but it > >> > isn't. > >> > >> Well, the point is that the commands it emits will cause the eventual > >> restore to go through the root. Anyway, I think trying to avoid using > >> a verb altogether is going to result in a very stilted option name. > >> > >> I notice that the option list already includes some references to > >> "insert", so maybe "--insert-via-partition-root"? Although you could > >> argue that that's confusing when we're using COPY. > > > > > > --use-partitioned-table [partition-name, ...] # if names are omitted it > > defaults to all partitioned tables > > I like this idea since it allows using this feature for selected > tables e.g. hash tables. Otherwise, users will be forced to use this > option even when there is only a single hash partitioned table and > many other list/range partitioned tables. > > +1. > What we are trying to do here is dump the data in a partitioned table > as if it's not partitioned. Combine that with --data-only dumps, and > one could use it to load partitioned data into unpartitioned or > differently partitioned table. So, how about naming the option as > > --unpartition-partitioned-table [partitioned-table-name, ] # if > names are omitted it defaults to all the partitioned tables. > > --unpartition-partitioned-table is very confusing. I liked the previous option which is --use-partitioned-table [partition-name, ...]. The Only problem with --use-partitioned-table is, a user needs to specify the partition-name in the options list. Imagine if someone having 100's of partitions then specifying those name is pg_dump option is a pain. Rather than that: --use-partitioned-table [partitioned_name, ...] # if names are omitted it defaults to all the partitioned tables. Here user need to specify the root relation name in the option - and any partition table have that as a ROOT, will load the data through top-parent-relation. That really says what dump is really doing without focusing on how the > data will be used like restoring/inserting/copying etc. > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > -- Rushabh Lathia
[HACKERS] Unused field of ProjectionPath
I've noticed that the dummypp field of ProjectionPath is set but never read.
If the only possible change between the path and plan creation time is that
the projection path and the subpath targetlists become different, then dummypp
could be used this way:
diff --git a/src/backend/optimizer/plan/createplan.c
b/src/backend/optimizer/plan/createplan.c
new file mode 100644
index 5c934f2..1910d3f
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
*** create_projection_plan(PlannerInfo *root
*** 1572,1578
* not using.)
*/
if (is_projection_capable_path(best_path->subpath) ||
! tlist_same_exprs(tlist, subplan->targetlist))
{
/* Don't need a separate Result, just assign tlist to subplan */
plan = subplan;
--- 1572,1578
* not using.)
*/
if (is_projection_capable_path(best_path->subpath) ||
! (best_path->dummypp && tlist_same_exprs(tlist,
subplan->targetlist)))
{
/* Don't need a separate Result, just assign tlist to subplan */
plan = subplan;
On the other hand, if the targetlists can also be different at path creation
time and equal at plan creation time, the lists do always need comparison at
plan creation time and the dummypp field should probably be removed.
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at
--
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] Support for Secure Transport SSL library on macOS as OpenSSL alternative
> On 03 Aug 2017, at 13:06, Heikki Linnakangas wrote: > > On 08/03/2017 01:02 PM, Daniel Gustafsson wrote: >> >> The frontend has support for using PEM files for certificates and keys. It >> can >> also look up the key for the certificate in a Keychain, or both certificate >> and >> key in a Keychain. The root certificate is still just read from a PEM file. > > Why can't you have the root certificate in the keychain, too? Just not > implemented yet, or is there some fundamental problem with it? Just not implemented yet, awaiting Keychain discussions. >> The existence of an sslcert config trumps a keychain, but when a keychain is >> used I’m currently using the sslcert parameter (awaiting a discussion on how >> to >> properly do this) for the certificate label required to search the keychain. >> There is a new frontend parameter called “keychain” with which a path to a >> custom Keychain file can be passed. If set, this Keychain will be searched >> as >> well as the default. If not, only the default user Keychain is used. There >> is >> nothing that modifies the Keychain in this patch, it can read identities >> (certificates and its key) but not alter them in any way. > > OpenSSL also has a mechanism somewhat similar to the keychain, called > "engines". You can e.g. keep the private key corresponding a certificate on a > smart card, and speak to it with an OpenSSL "smart card reader" engine. If > you do that, the 'sslkey' syntax is ":". Perhaps we > should adopt that syntax here as well? For example, to read the client > certificate from the key chain, you would use libpq options like > "keychain=/home/heikki/my_keychain sslcert=keychain:my_client_cert”. Thats definitely an option, although it carries a bit redudancy in this case which can confuse users. With “keychain=/foo/bar.keychain sslcert=my_cert”, did the user mean a file called my_cert or is it a reference to a cert in the keychain? Nothing that strict parsing rules, good errormessages and documentation can’t solve but needs careful thinking. >> “keychain” is obviously a very Secure Transport specific name, and I >> personally >> think we should avoid that. Any new configuration added here should take >> future potential implementation into consideration such that avoid the need >> for >> lots of backend specific knobs. “sslcertstore” comes to mind as an >> alternative, but we’d also need parameters to point into the certstore for >> finding what we need. Another option would be to do a URL based scheme >> perhaps. > > I wouldn't actually mind using implementation-specific terms like "keychain" > here. It makes it clear that it's implementation-specific. I think it would > be confusing, to use the same generic option name, like "sslcertstore", for > both a macOS keychain and e.g. the private key store in Windows. Or GNU > Keyring. In the worst case, you might even have multiple such "key stores" on > the same system, so you'd anyways need a way to specify which one you mean. Thats a good point. > Actually, perhaps it should be made even more explicit, and call it > "secure_transport_keychain". That's quite long, though. Yeah, secure_transport_ is a pretty long prefix. > Wrt. keychains, is there a system-global or per-user keychain in macOS? And > does this patch use them? If you load a CRL into a global keychain, does it > take effect? Each user has a default Keychain , and on top of that you can create as many Keychains as you want (a Keychain is really just a database file containing secret things). The frontend use the default one for lookups unless the keychain parameter is set in which case it uses both. When evaluating trust, Secure Transport will use the default Keychain even if not explicitly opened (as in the backend code). It does however only search for intermediate certificates and not root certs/CRLs. Adding a policy object for using CRLs should force it to afaik, but we’d need to support additional keychains (if only to be able to test without polluting the default). >> Testing >> === >> In order to test this we need to provide an alternative to the openssl calls >> in >> src/test/ssl/Makefile for Secure Transport. > > Those openssl commands are only needed to re-generate the test certificates. > The certificates are included in the git repository, so you only need to > re-generate them if you want to modify them or add new ones. I think it's OK > to require the openssl tool for that, it won't be needed just to run the test > suite. Ah, of course. We still need support for re-generating Keychains (or at all generate them in case we don’t want binaries in the repository). >> Documentation >> = >> I have started fiddling with this a little, but to avoid spending time on the >> wrong thing I have done very little awaiting the outcome of discussions here. >> I have tried to add lots of comments to the code however, to explain the >> quirks >> of Secure Transport.
Re: [HACKERS] Improve the performance of the standby server when dropping tables on the primary server
Hi, > * The previous coding allowed for a fast path to access the last > unowned relation, which this patch removes. It seems a good idea to > cache the last unowned relation, or if not explain why the comment > that says why it worked that way is no longer true. > > * We should only create the hash table when needed, i.e. on or after > when we add an unowned relation, since that is not a typical case. > > * The hash table is sized at 400 elements and will grow from there. > The comments in dynahash say "An overly large nelem will penalize > hash_seq_search speed without buying much." so this makes your patch > suitable for the bulk case but likely to perform worse for fewer > elements. So I'm guessing that you picked 400 because that's what the > parameter is set to for the smgr relation table rather than because > this has had good consideration. I thought abount improving the above problems. But I have no good ideas to improve every case. Do you have any good ideas? I suggest to apply this modification only for the startup process. This is because the startup process has many unowned SMgrRelation objects. In XLOG replay, statup process create fake relcaches. Fake relcaches create unowned SMgrRelation objects. So startup process has more unowned SMgrRelation objects than any other process. Of cource, it is necessary to think about the problems such as hash size. Do you think about it. Regards, Takashi Tokuda -- 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] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On 08/03/2017 01:02 PM, Daniel Gustafsson wrote: In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I presented a WIP patch for adding support for the Apple Secure Transport SSL library on macOS as, an alternative to OpenSSL. That patch got put on the backburner for a bit, but I’ve now found the time to make enough progress to warrant a new submission for discussions on this (and hopefully help hacking). Hooray! Keychains = The frontend has support for using PEM files for certificates and keys. It can also look up the key for the certificate in a Keychain, or both certificate and key in a Keychain. The root certificate is still just read from a PEM file. Why can't you have the root certificate in the keychain, too? Just not implemented yet, or is there some fundamental problem with it? The existence of an sslcert config trumps a keychain, but when a keychain is used I’m currently using the sslcert parameter (awaiting a discussion on how to properly do this) for the certificate label required to search the keychain. There is a new frontend parameter called “keychain” with which a path to a custom Keychain file can be passed. If set, this Keychain will be searched as well as the default. If not, only the default user Keychain is used. There is nothing that modifies the Keychain in this patch, it can read identities (certificates and its key) but not alter them in any way. OpenSSL also has a mechanism somewhat similar to the keychain, called "engines". You can e.g. keep the private key corresponding a certificate on a smart card, and speak to it with an OpenSSL "smart card reader" engine. If you do that, the 'sslkey' syntax is ":name>". Perhaps we should adopt that syntax here as well? For example, to read the client certificate from the key chain, you would use libpq options like "keychain=/home/heikki/my_keychain sslcert=keychain:my_client_cert". “keychain” is obviously a very Secure Transport specific name, and I personally think we should avoid that. Any new configuration added here should take future potential implementation into consideration such that avoid the need for lots of backend specific knobs. “sslcertstore” comes to mind as an alternative, but we’d also need parameters to point into the certstore for finding what we need. Another option would be to do a URL based scheme perhaps. I wouldn't actually mind using implementation-specific terms like "keychain" here. It makes it clear that it's implementation-specific. I think it would be confusing, to use the same generic option name, like "sslcertstore", for both a macOS keychain and e.g. the private key store in Windows. Or GNU Keyring. In the worst case, you might even have multiple such "key stores" on the same system, so you'd anyways need a way to specify which one you mean. Actually, perhaps it should be made even more explicit, and call it "secure_transport_keychain". That's quite long, though. Wrt. keychains, is there a system-global or per-user keychain in macOS? And does this patch use them? If you load a CRL into a global keychain, does it take effect? Testing === In order to test this we need to provide an alternative to the openssl calls in src/test/ssl/Makefile for Secure Transport. Those openssl commands are only needed to re-generate the test certificates. The certificates are included in the git repository, so you only need to re-generate them if you want to modify them or add new ones. I think it's OK to require the openssl tool for that, it won't be needed just to run the test suite. Documentation = I have started fiddling with this a little, but to avoid spending time on the wrong thing I have done very little awaiting the outcome of discussions here. I have tried to add lots of comments to the code however, to explain the quirks of Secure Transport. I think this patch in general is in very good shape, and the next step is to write the documentation. In particular, I'd like to see documentation on how the keychain stuff should work. It'll be easier to discuss the behavior and the interactions, once it's documented. In fact, better to write the documentation for that now, and not bother to change the code, until after we've discussed and are happy with the documented behavior. I went into this thinking I would write a README for how to implement a new SSL library. But in the end, turns out the refactoring that went into our SSL code earlier made that part almost too easy to warrant that. It’s really quite straightforward. That's nice to hear! - Heikki -- 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] Change in "policy" on dump ordering?
Am Donnerstag, den 27.07.2017, 15:52 -0400 schrieb Tom Lane: > So I'm thinking we should consider the multi-pass patch I posted as > a short-term, backpatchable workaround, which we could hope to > replace with dependency logic later. +1, it would be really nice if this could be fixed in the back-branches before v11. Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.ba...@credativ.de credativ GmbH, HRB Mönchengladbach 12080 USt-ID-Nummer: DE204566209 Trompeterallee 108, 41189 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reload-through-the-top-parent switch the partition table
On Wed, Aug 2, 2017 at 11:47 PM, David G. Johnston wrote: > On Wed, Aug 2, 2017 at 10:58 AM, Tom Lane wrote: >> >> Robert Haas writes: >> > On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane wrote: >> >> --restore-via-partition-root ? >> >> > I worry someone will think that pg_dump is now restoring stuff, but it >> > isn't. >> >> Well, the point is that the commands it emits will cause the eventual >> restore to go through the root. Anyway, I think trying to avoid using >> a verb altogether is going to result in a very stilted option name. >> >> I notice that the option list already includes some references to >> "insert", so maybe "--insert-via-partition-root"? Although you could >> argue that that's confusing when we're using COPY. > > > --use-partitioned-table [partition-name, ...] # if names are omitted it > defaults to all partitioned tables I like this idea since it allows using this feature for selected tables e.g. hash tables. Otherwise, users will be forced to use this option even when there is only a single hash partitioned table and many other list/range partitioned tables. What we are trying to do here is dump the data in a partitioned table as if it's not partitioned. Combine that with --data-only dumps, and one could use it to load partitioned data into unpartitioned or differently partitioned table. So, how about naming the option as --unpartition-partitioned-table [partitioned-table-name, ] # if names are omitted it defaults to all the partitioned tables. That really says what dump is really doing without focusing on how the data will be used like restoring/inserting/copying etc. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transactions involving multiple postgres foreign servers
On Mon, Jul 31, 2017 at 7:27 PM, Alvaro Herrera wrote: > Robert Haas wrote: > >> An alternative approach is to have some kind of other identifier, >> let's call it a distributed transaction ID (DXID) which is mapped by >> each node onto a local XID. > > Postgres-XL seems to manage this problem by using a transaction manager > node, which is in charge of assigning snapshots. I don't know how that > works, but perhaps adding that concept here could be useful too. One > critical point to that design is that the app connects not directly to > the underlying Postgres server but instead to some other node which is > or connects to the node that manages the snapshots. > > Maybe Michael can explain in better detail how it works, and/or how (and > if) it could be applied here. XL (and XC) use a transaction ID that plugs in directly with the internal XID assigned by Postgres, actually bypassing what Postgres assigns to each backend if a transaction needs one. So if transactions are not heavenly shared among multiple nodes, performance gets impacted. Now when we worked on this project we noticed that we gained in performance by reducing the number of requests and grouping them together, so a proxy layer has been added between the global transaction manager and Postgres to group those requests. This does not change the fact that read-committed transactions still need snapshots for each query, which is consuming. So this approach hurts less with analytic queries, and more with OLTP. 2PC transaction status was tracked as well in the GTM. This allows fancy things like being able to prepare a transaction on node 1, and commit it on node 2 for example. I am not honestly sure that you need to add anything at clog level for example, but I think that having at the FDW level the meta data of a transaction stored as a rather correct approach on the matter. That's what greenplum actually does if I recall correctly (Heikki save me!): it has one coordinator with such metadata handling, and bunch of underlying nodes that store the data. Citus does also that if I recall correctly. So instead of decentralizing this information, this gets stored in a Postgres coordinator instance. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I presented a WIP patch for adding support for the Apple Secure Transport SSL library on macOS as, an alternative to OpenSSL. That patch got put on the backburner for a bit, but I’ve now found the time to make enough progress to warrant a new submission for discussions on this (and hopefully help hacking). It is a drop-in replacement for the OpenSSL code, and supports all the same features and options, except for two things: compression is not supported and the CRL cannot be loaded from a plain PEM file. A Keychain must be used for that instead. Current state = The frontend and backend are implemented more or less fully, the two main things missing being the CRL support (further details below) and loading DH files (to support the GUC added in c0a15e07cd). All non-CRL tests but one are passing. The failing test is in the frontend and it also fails when running against an OpenSSL backend, but I can’t find where the logic is flawed and could do with some help there. Threads === Just like the CFLocaleCopyCurrent() call referenced in postmaster.c, the Secure Transport APIs makes the process multithreaded. To keep this out of the postmaster, and contained in the backend, I’ve moved all functionality to open_server and left init empty. I could definitely need some clues on how to properly handle this, or if it’s a complete showstopper. Keychains = The frontend has support for using PEM files for certificates and keys. It can also look up the key for the certificate in a Keychain, or both certificate and key in a Keychain. The root certificate is still just read from a PEM file. The existence of an sslcert config trumps a keychain, but when a keychain is used I’m currently using the sslcert parameter (awaiting a discussion on how to properly do this) for the certificate label required to search the keychain. There is a new frontend parameter called “keychain” with which a path to a custom Keychain file can be passed. If set, this Keychain will be searched as well as the default. If not, only the default user Keychain is used. There is nothing that modifies the Keychain in this patch, it can read identities (certificates and its key) but not alter them in any way. The backend is only supporting PEM files at this point. Once we have support for Keychains, we can however use them for additionally supporting other Secure Transport features like OCSP etc. “keychain” is obviously a very Secure Transport specific name, and I personally think we should avoid that. Any new configuration added here should take future potential implementation into consideration such that avoid the need for lots of backend specific knobs. “sslcertstore” comes to mind as an alternative, but we’d also need parameters to point into the certstore for finding what we need. Another option would be to do a URL based scheme perhaps. Certificate Revocation == Secure Transport supports loading CRL lists into Keychain files, the command line certtool can for example do that. When doing the trust evaluation on the connection, a policy can be added which enables revocation checking via CRL. I have however been unable to figure out how to load a CRL list programmatically, and as far as I can tell there is no API for this. The certtool utility is using the low-level CSSM APIs for this it seems, but adding that level of complexity doesn’t seem worth it for us to maintain (I did a test and it turned big, ugly and messy). Unless someone knows how to implement this, we’d be limited to requiring the use of a Keychain file for CRL support. This would break drop-in replacement support, but supporting certificate revocation seems more important. Platform Support I’ve tested this on 10.11 El Capitan and 10.12 Sierra, which are the systems I have access to. Supporting prairiedog and dromedary in the buildfarm will probably be too hard (if at all possible), but down to 10.9 Mavericks should be quite possible (if someone has the required systems to test on). It adds a dependency on Core Foundation on top of Secure Transport, no other macOS APIs are used. Testing === In order to test this we need to provide an alternative to the openssl calls in src/test/ssl/Makefile for Secure Transport. On top of that, code to generate Keychains is required. The certtool application can do all the Keychain generations (I think) but this is still left open. The main thing to figure out here is how to avoid having to type in the Keychain password in a modal GUI that Secure Transport pops up. Since a Keychain can be passwordless it should be doable, but the right certtool incantations for that is still evading me. I’ve included a show-and-tell patch for this which I’ve used locally for testing during hacking. Documentation = I have started fiddling with this a little, but to avoid spending time on the
Re: [HACKERS] Update comments in nodeModifyTable.c
On 2017/08/02 4:07, Robert Haas wrote:
On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
wrote:
Maybe I'm missing something, but I'm not sure that's a good idea because the
change says like we might have 'wholerow' only for the FDW case, but that
isn't correct because we would have 'wholerow' for a view as well. ISTM that
views should be one of the typical cases, so I'd like to propose to modify
the sentence starting with 'Typically' to something like this: "Typically,
this will be a 'ctid' or 'wholerow' attribute, but in the case of a foreign
data wrapper it might be a set of junk attributes sufficient to identify the
remote row." What do you think about that?
Works for me.
I updated the patch that way. Attached is a new version of the patch.
Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 0dde0ed..0199c9d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1696,7 +1696,7 @@ ExecModifyTable(PlanState *pstate)
* the old relation tuple.
*
* Foreign table updates have a wholerow
attribute when the
-* relation has an AFTER ROW trigger. Note
that the wholerow
+* relation has a row-level trigger. Note that
the wholerow
* attribute does not carry system columns.
Foreign table
* triggers miss seeing those, except that we
know enough here
* to set t_tableOid. Quite separately from
this, the FDW may
@@ -2182,8 +2182,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
/*
* Initialize the junk filter(s) if needed. INSERT queries need a
filter
* if there are any junk attrs in the tlist. UPDATE and DELETE always
-* need a filter, since there's always a junk 'ctid' or 'wholerow'
-* attribute present --- no need to look first.
+* need a filter, since there's always at least one junk attribute
present
+* --- no need to look first. Typically, this will be a 'ctid' or
+* 'wholerow' attribute, but in the case of a foreign data wrapper it
+* might be a set of junk attributes sufficient to identify the remote
+* row.
*
* If there are multiple result relations, each one needs its own junk
* filter. Note multiple rels are only possible for UPDATE/DELETE, so
we
@@ -2251,7 +2254,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
else if (relkind ==
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER
trigger, there should be a
+* When there is a row-level
trigger, there should be a
* wholerow attribute.
*/
j->jf_junkAttNo =
ExecFindJunkAttribute(j, "wholerow");
--
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] pgbench: Skipping the creating primary keys after initialization
Hello,
My motivation of this proposal is same as what Robert has. I
understand that ad-hoc option can solve only the part of big problem
and it could be cause of mess. However It seems me that the script
especially for table initialization will not be flexible than we
expected. I mean, even if we provide some meta commands for table
initialization or data loading, these meta commands work for only
pgbench tables (i.g., pgbench_accounts, pgbench_branches and so on).
If we want to create other tables and load data to them as we want we
can do that using psql -f. So an alternative ways is having a flexible
style option for example --custom-initialize = { [load, create_pkey,
create_fkey, vacuum], ... }. That would solve this in a better way.
Personnaly, I could be fine with a limited number of long options to
adjust pgbench initialization to various needs, eg --use-hash-index,
--skip-whetever-index, etc.
The flexible --custom-init idea outlined above looks nice as well.
As for a more generic solution, the easy part are the "CREATE" stuff and
the transaction script stuff (existing pgbench scripts).
For the CREATE stuff, the script language is SQL, the command to use it is
"psql"...
The real and hard part is to fill tables with meaningful pseudo-random
test data which do not violate constraints for any non trivial schema
involving foreign keys and various unique constraints.
The solution for this is SQL for trivial cases, think of:
"INSERT INTO Foo() SELECT ... FROM generate_series(...);"
For instance the pgbench initialization is really close to:
psql -Dscale=10
Re: [HACKERS] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)
On Mon, Jul 31, 2017 at 6:13 PM, Robert Haas wrote: > On Fri, Jul 28, 2017 at 10:35 AM, Andreas Joseph Krogh > wrote: >> I'm reading https://www.postgresql.org/docs/10/static/pgupgrade.html to try >> to understand how to upgrade standby-servers using pg_upgrade with pg10. >> >> The text in step 10 sais: >> "You will not be running pg_upgrade on the standby servers, but rather >> rsync", which to me sounds like rsync, in step 10-f, should be issued on the >> standy servers. Is this the case? If so I don't understand how the standby's >> data is upgraded and what "remote_dir" is. If rsync is supposed to be issued >> on the primary then I think it should be explicitly mentioned, and step 10-f >> should provide a clarer example with more detailed values for the >> directory-structures involved. >> >> I really think section 10 needs improvement as I'm certainly not comfortable >> upgrading standbys following the existing procedure. > > Yeah, I don't understand it either, and I have never been convinced > that there's any safe way to do it other than recloning the standbys > from the upgraded master. Here are my 2c on the matter. 10-f means that the upgraded node may have generated WAL with wal_level = minimal, which, at least it seems to me, that we have a risk of having inconsistent data pages if only a rsync is used on the old standbys. Like Robert, the flow we used in the products I work on is to re-create standbys from scratch after the upgrade using a fresh backup, with a VM cloning. An upgrade here is an in-place process not only linked to Postgres, so standby VMs are made of many services, some are being linked to Postgres. So this choice is mainly decided by those dependencies, still it feels safer anyway. -- 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] Confusing error message in pgbench
Indeed. It doesn't look that hard: AFAICS the problem is just that process_sql_command() is making premature decisions about whether to extract parameters from the SQL commands. Proposed patch attached. Great. Patch looks good to me. Too me as well: code looks ok, patch applies, compiles, make check ok, manual tests with pgbench ok. That is one more patch about pgbench in the queue. -- Fabien. -- 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 and partitioned tables
Thanks Amit for addressing the comment. The patch looks good to me. I have no more comments. Verified that v2 patch applies cleanly and make check passes. Thanks, Jeevan Ladhe
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/03 17:12, Amit Langote wrote: Attached updated patches. Thanks for the patch! That looks good to me. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Red-Black tree traversal tests
Hi Victor, > I forgot to attach the patch. Sorry. > Here it is. I would say that this patch is in a pretty good shape now. And I see a 99% code coverage of rbtree.c. Let's see what committers think. -- Best regards, Aleksander Alekseev signature.asc Description: PGP signature
Re: [HACKERS] map_partition_varattnos() and whole-row vars
Fujita-san,
Thanks for the review.
On 2017/08/03 16:01, Etsuro Fujita wrote:
> On 2017/08/02 15:21, Amit Langote wrote:
>> On 2017/08/02 1:33, Amit Khandekar wrote:
>>> ---
>>>
>>> Few more comments :
>>>
>>> @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
>>> var->varlevelsup == context->sublevels_up)
>>> {
>>> /* Found a matching variable, make the substitution */
>>>
>>> - Var*newvar = (Var *) palloc(sizeof(Var));
>>> + Var*newvar = copyObject(var);
>>>int attno = var->varattno;
>>>
>>> *newvar = *var;
>>>
>>> Here, "*newvar = *var" should be removed.
>>
>> Done.
>
> I'm not sure this change is a good idea, because the copy by "*newvar =
> *var" would be more efficient than the copyObject(). (We have this
> optimization in other places as well. See eg, copyVar() in setrefs.c.)
OK, done.
> Here are some other comments:
>
> +/* If the callers expects us to convert the same, do so. */
> +if (OidIsValid(context->to_rowtype))
> +{
> +ConvertRowtypeExpr *r;
> +
> +/* Var itself is converted to the requested rowtype. */
> +newvar->vartype = context->to_rowtype;
> +
> +/*
> + * And a conversion step on top to convert back to the
> + * original type.
> + */
> +r = makeNode(ConvertRowtypeExpr);
> +r->arg = (Expr *) newvar;
> +r->resulttype = var->vartype;
> +r->convertformat = COERCE_IMPLICIT_CAST;
> +r->location = -1;
> +
> +return (Node *) r;
> +}
>
> Why not do this conversion if to_rowtype is valid and it's different from
> the rowtype of the original whole-row Var like the previous patch? Also, I
> think it's better to add an assertion that the rowtype of the original
> whole-row Var is a named one. So, what I have in mind is:
>
> if (OidIsValid(context->to_rowtype))
> {
> Assert(var->vartype != RECORDOID)
> if (var->vartype != context->to_rowtype)
> {
> ConvertRowtypeExpr *r;
>
> /* Var itself is converted to the requested rowtype. */
> ...
> /* And a conversion step on top to convert back to the ... */
> ...
> return (Node *) r;
> }
> }
Sounds good, so done.
> Here is the modification to the map_variable_attnos()'s API:
>
> map_variable_attnos(Node *node,
> int target_varno, int sublevels_up,
> const AttrNumber *attno_map, int
> map_length,
> - bool *found_whole_row)
> + bool *found_whole_row, Oid
> to_rowtype)
>
> This is nitpicking, but I think it would be better to put the new argument
> to_rowtype right before the output argument found_whole_row.
I consider this a good suggestion. I guess we tend to list all the input
arguments before any output arguments. So done as you suggest.
> + * RelationGetRelType
> + *Returns the rel's pg_type OID.
> + */
> +#define RelationGetRelType(relation) \
> +((relation)->rd_rel->reltype)
>
> This macro is used in only one place. Do we really need that? (This
> macro would be useful for other places such as expand_inherited_rtentry,
> but I think it's better to introduce this in a separate patch, maybe for
> PG11.)
Alright, dropped RelationGetRelType from the patch.
> +-- check that wholerow vars in the RETUNING list work with partitioned
> tables
>
> Typo: s/RETUNING/RETURNING/
Fixed.
Attached updated patches.
Thanks,
Amit
From 9b2d16ec4c8eadd7261849d5aa0f34ee2577b405 Mon Sep 17 00:00:00 2001
From: amit
Date: Wed, 26 Jul 2017 16:45:46 +0900
Subject: [PATCH 1/2] Fix map_partition_varattnos to not error on
found_whole_row
It was designed assuming that the expressions passed to it can never
contain whole-row vars, but it's wrong since it's called from places
that pass it WCO constraint expressions and RETURNING target list
expressions, which can very well contain whole-row vars.
Move the responsibility of error'ing out to the callers, because they
are in position to know that finding whole-row vars is an error
condition.
Adds test in insert.sql and updatable_views.sql.
Reported by: Rajkumar Raghuwanshi
Report:
https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com
---
src/backend/catalog/partition.c | 20 ++--
src/backend/commands/tablecmds.c | 8 +++-
src/backend/executor/nodeModifyTable.c| 18 ++
src/include/catalog/partition.h | 3 ++-
src/test/regress/expected/insert.out | 10 ++
src/test/regress/expected/updatable_views.out | 10 ++
src/test/regress/sql/insert.sql
Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server
On Thu, Aug 3, 2017 at 4:29 AM, Stephen Frost wrote: > I'll provide another update tomorrow. Hopefully Michael is able to produce > a 9.6 patch, otherwise I'll do it. I have sent an updated version of the patch, with something that can be used for 9.6 as well. It would be nice to get something into the next set of minor releases. -- 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] pg_stop_backup(wait_for_archive := true) on standby server
On Wed, Aug 2, 2017 at 7:58 PM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> Do you need a back-patchable version for 9.6? I could get one out of >> my pocket if necessary. > > I was just trying to find a bit of time to generate exactly that- if > you have a couple spare cycles, it would certainly help. OK, here you go. Even if archive_mode = always has been introduced in 9.5, but non-exclusive mode is a 9.6 feature, so here are patches down to this version. I am pretty satisfied by this, and I included all the comments and error message corrections reviewed up to now. I noticed some incorrect comments, doc typos and an incorrect indentation as well for the WARNING reported to the user when waiting for the archiving. -- Michael pg_stop_backup_on_standby_v6_96.patch Description: Binary data pg_stop_backup_on_standby_v6_master.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] map_partition_varattnos() and whole-row vars
On 2017/08/02 15:21, Amit Langote wrote:
Thanks Fuita-san and Amit for reviewing.
On 2017/08/02 1:33, Amit Khandekar wrote:
On 1 August 2017 at 15:11, Etsuro Fujita wrote:
On 2017/07/31 18:56, Amit Langote wrote:
Yes, that's what's needed here. So we need to teach
map_variable_attnos_mutator() to convert whole-row vars just like it's
done in adjust_appendrel_attrs_mutator().
Seems reasonable. (Though I think it might be better to do this kind of
conversion in the planner, not the executor, because that would increase the
efficiency of cached plans.)
That's a good point, although it sounds like a bigger project that, IMHO,
should be undertaken separately, because that would involve designing for
considerations of expanding inheritance even in the INSERT case.
Agreed. I think that would be definitely a material for PG11.
I think the work of shifting to planner should be taken as a different
task when we shift the preparation of DispatchInfo to the planner.
Yeah, I think it'd be a good idea to do those projects together. That is,
doing what Fujita-san suggested and expanding partitioned tables in
partition bound order in the planner.
OK
---
Few more comments :
@@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
var->varlevelsup == context->sublevels_up)
{
/* Found a matching variable, make the substitution */
- Var*newvar = (Var *) palloc(sizeof(Var));
+ Var*newvar = copyObject(var);
int attno = var->varattno;
*newvar = *var;
Here, "*newvar = *var" should be removed.
Done.
I'm not sure this change is a good idea, because the copy by "*newvar =
*var" would be more efficient than the copyObject(). (We have this
optimization in other places as well. See eg, copyVar() in setrefs.c.)
Here are some other comments:
+ /* If the callers expects us to convert the
same, do so. */
+ if (OidIsValid(context->to_rowtype))
+ {
+ ConvertRowtypeExpr *r;
+
+ /* Var itself is converted to the
requested rowtype. */
+ newvar->vartype = context->to_rowtype;
+
+ /*
+* And a conversion step on top to
convert back to the
+* original type.
+*/
+ r = makeNode(ConvertRowtypeExpr);
+ r->arg = (Expr *) newvar;
+ r->resulttype = var->vartype;
+ r->convertformat = COERCE_IMPLICIT_CAST;
+ r->location = -1;
+
+ return (Node *) r;
+ }
Why not do this conversion if to_rowtype is valid and it's different
from the rowtype of the original whole-row Var like the previous patch?
Also, I think it's better to add an assertion that the rowtype of the
original whole-row Var is a named one. So, what I have in mind is:
if (OidIsValid(context->to_rowtype))
{
Assert(var->vartype != RECORDOID)
if (var->vartype != context->to_rowtype)
{
ConvertRowtypeExpr *r;
/* Var itself is converted to the requested rowtype. */
...
/* And a conversion step on top to convert back to the ... */
...
return (Node *) r;
}
}
Here is the modification to the map_variable_attnos()'s API:
map_variable_attnos(Node *node,
int target_varno, int sublevels_up,
const AttrNumber *attno_map,
int map_length,
- bool *found_whole_row)
+ bool *found_whole_row, Oid
to_rowtype)
This is nitpicking, but I think it would be better to put the new
argument to_rowtype right before the output argument found_whole_row.
+ * RelationGetRelType
+ * Returns the rel's pg_type OID.
+ */
+#define RelationGetRelType(relation) \
+ ((relation)->rd_rel->reltype)
This macro is used in only one place. Do we really need that? (This
macro would be useful for other places such as expand_inherited_rtentry,
but I think it's better to introduce this in a separate patch, maybe for
PG11.)
+-- check that wholerow vars in the RETUNING list work with partitioned
tables
Typo: s/RETUNING/RETURNING/
Sorry for the delay.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
