Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
On Thu, Aug 3, 2017 at 2:00 AM, Robert Haas wrote:
> On Wed, Aug 2, 2017 at 12:34 PM, Tom Lane wrote:
>> Of course. It's also a heck of a lot more flexible. Adding on another
>> ad-hoc option that does the minimum possible amount of work needed to
>> address one specific problem is always going to be less work; but after
>> we repeat that process five or ten times, we're going to have a mess.
>
> Well, I still like Masahiko-san's proposal, but I'm not prepared to
> keep arguing about it right now. Maybe some other people will weigh
> in with an opinion.
>
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.
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] On Complex Source Code Reading Strategy
On Wed, Aug 2, 2017 at 9:30 PM, Michael Paquier wrote: > On Wed, Aug 2, 2017 at 7:24 AM, Zeray Kalayu wrote: >> Lastly, I strongly believe that Code is the ultimate truth and being >> able to understand complex and high quality code effectively and >> strategically is of paramount importance. > > Documentation to understand how a system works from the user > prospective, and comments in the code itself are also important > properties of a code that can be considered as a good base. Postgres > has both. Michael,thanks and I am trying to take advantage of them. Actually, I have learned that being hacker of PG or other data management platforms like Greenplum is not a one-stop story or journey. It is the sum of deep understanding of computing sciences(the foundations like compatibility theory, computational complexity...), the engineering aspect of computing and the tech aspect of the same. 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. Regards, Zeray -- 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 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. > > However, if there are multiple places that should be kept in sync > regarding which relkinds to check, then I don't understand Robert's > objection that only one place requires the check. Surely we're having > this discussion precisely because more than one place needs the check, > and finding those places is not obvious? > A new relkind may be required to be added in tests at multiple places. But each place may have different set of relkinds in that test, which gets converted into a macro. Given that we have now 9 relkinds, we could theoretically have 2^9 tests and those many macros. Maintaining all those would be more cumbersome than grepping RELKIND_ in code. >From that perspective Robert's concern is valid, but my feeling is that we are actually using far lesser combinations than that. As I said, I haven't gone through all the places, so, I don't know the whole picture yet. But given the number of places where we added RELKIND_PARTITIONED_TABLE, I guess, there are more than one places where at least some of those tests are used. -- 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] PostgreSQL not setting OpenSSL session id context?
One more note: https://github.com/netty/netty/pull/5321/files is an equivalent PR setting the session ID context to a constant value in netty (which is also a server using OpenSSL). This is in line with the documentation on SSL_CTX_set_session_id_context ( https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_session_id_context(3) ): > Sessions are generated within a certain context. When exporting/importing sessions with *i2d_SSL_SESSION*/*d2i_SSL_SESSION* it would be possible, to re-import a session generated from another context (e.g. another application), which might lead to malfunctions. Therefore each application must set its own session id context *sid_ctx* which is used to distinguish the contexts and is stored in exported sessions. The *sid_ctx* can be any kind of binary data with a given length, it is therefore possible to use e.g. the name of the application and/or the hostname and/or service name ...
Re: [HACKERS] PostgreSQL not setting OpenSSL session id context?
> > Shay Rojansky writes: > > Once again, I manged to make the error go away simply by setting the > > session id context, which seems to be a mandatory server-side step for > > properly support session tickets. > > The fact that you made the error go away doesn't make this a good > solution. In particular, using a simple constant session ID is completely > insecure according to the TLS spec. RFC 5246, F.1.4, doesn't even care > for the idea of ever writing session IDs to stable storage; although > Apache seems to be content with a session ID that is unique per-server > (it looks like they just use a hash of the server's host name). > I think there may be a confusion here - I'm not doing anything with session IDs, merely setting the session ID *context*. This seems to be an OpenSSL-specific feature (nothing to do with TLS) that simply allows distinguishing between different "applications" (or contexts) running in the same process. See https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_session_id_context(3) for the docs. This feature does not involve writing anything (and definitely not session IDs) to stable storage. The idea is to provide the client with an opaque "session ticket", which is passed by the client back to the server on subsequent connections, and which allows the skipping of a roundtrip in the SSL handshake. > > More generally, PG as currently configured can't do anything with a > session cache since each new backend would start with an empty cache. > Again, there's no backend cache - RFC5077 is about having all state at the client side. > So the question here is whether it's safe or worthwhile to allow use > of session tickets. I agree with Heikki's opinion that it's unlikely > to provide any meaningful performance gain for database sessions that > are of reasonable length. I'm also pretty concerned about the possibility > for security problems, eg a client being able to force a server into some > low-security SSL mode. Both RFC 5077 and the Apache people say that if > you use session tickets you'd better rotate the keys for them regularly, > eg in Apache's changelog we find > > Session ticket creation uses a random key created during web > server startup and recreated during restarts. No other key > recreation mechanism is available currently. Therefore using session > tickets without restarting the web server with an appropriate > frequency > (e.g. daily) compromises perfect forward secrecy. [Rainer Jung] > > Since we have no mechanism for that, I think that we need to err on > the side of security. > I may definitely be wrong about this, but I'm under the impression that management of the session ticket (as of the entire resumption mechanism) is OpenSSL's responsibility and does not require anything from PostgreSQL itself. However, if you're suspicious of OpenSSL itself that's another story (and I'd definitely understand). > > Accordingly, what I think we should do is something more like the > attached. Could you see whether it fixes your problem? > I will be able to test this later tonight and confirm. I'm not sure why the SSL_OP_NO_TICKET is in an #ifdef, I would simply do it in all cases. I've seen people reporting that this issue is solved via setting SSL_OP_NO_TICKET (e.g. https://forums.aws.amazon.com/message.jspa?messageID=505895) so I'm not sure what SSL_CTX_set_session_cache_mode is supposed to add, but if the idea is to defensively disable other forms of caching than it makes sense. Just to be clear, I don't necessarily have a problem with disabling RFC5077 session resumption as the benefits in the PostgreSQL scenario aren't big (although I don't think a handshake roundtrip is completely negligible either). I just think it's advisable we understand exactly what it is we're disabling - there seems to be a confusion between session IDs, session ID contexts, server/client-side state etc.
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 3 August 2017 at 11:00, Amit Langote wrote: > Thanks for the review. > > On 2017/08/03 13:54, Amit Khandekar wrote: >> On 2 August 2017 at 11:51, Amit Langote wrote: >>> On 2017/08/02 1:33, Amit Khandekar wrote: Instead of callers of map_partition_varattnos() reporting error, we can have map_partition_varattnos() itself report error. Instead of the found_whole_row parameter of map_partition_varattnos(), we can have error_on_whole_row parameter. So callers who don't expect whole row, would pass error_on_whole_row=true to map_partition_varattnos(). This will simplify the resultant code a bit. >>> >>> So, I think it's only the callers of >>> map_partition_varattnos() who know whether finding whole-row vars is an >>> error and *what kind* of error. >> >> Ok. Got it. Thanks for the explanation. >> >> How about making found_whole_row as an optional parameter ? >> map_partition_varattnos() would populate it only if its not NULL. This >> way, callers who don't bother about whether it is found or not don't >> have to declare a variable and pass its address. In current code, >> ExecInitModifyTable() is the one who has to declare found_whole_row >> without needing it. > > Sure, done. Looks good. > But while implementing that, I avoided changing anything in > map_variable_attnos(), such as adding a check for not-NULLness of > found_whole_row. The only check added is in map_partition_varattnos(). Yes, I agree. That is anyway not relevant to the fix. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros bundling RELKIND_* conditions
On Wed, Aug 2, 2017 at 10:28 PM, Tom Lane wrote: > Robert Haas writes: >> I've thought about this kind of thing, too. But the thing is that >> most of these macros you're proposing to introduce only get used in >> one place. > > I think the value would be in having a centralized checklist of > things-to-fix-when-adding-a-new-relkind. right. > There's more than one way > to reach that goal, though. I wonder whether the task should be defined > more like "grep for 'RELKIND_' and fix every place you find that". That way one has to scan all code and change many files. Having them centrally at one place reduces that pain. > If there are places to touch that fail to mention that string, fix > them, using comments if nothing else. I didn't get this. > (But see fe797b4a6 and > followon commits for other solutions.) That and the follow-on commits replace hard-coded relkind values by corresponding macro. Though that work it itself is important, I do not see how that helps to find all the places where the new relkind added needs to be checked. > >> I think this might cause some problems for translators. > > Yeah, the error messages that list a bunch of different relkinds in text > form are going to be a hassle no matter what. Most of the ways you might > think of to change that will violate our translatability rules. > Ok. I agree with that. May be for now, we shouldn't touch the error messages at all. -- 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] Macros bundling RELKIND_* conditions
On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas wrote: > On Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat > wrote: >>> I noticed, that >>> after we introduced RELKIND_PARTITIONED_TABLE, we required to change a >>> number of conditions to include this relkind. We missed some places in >>> initial commits and fixed those later. I am wondering whether we >>> should creates macros clubbing relevant relkinds together based on the >>> purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind >>> is added, one can examine these macros to check whether the new >>> relkind fits in the given macro. If all those macros are placed >>> together, there is a high chance that we will not miss any place in >>> the initial commit itself. > > I've thought about this kind of thing, too. But the thing is that > most of these macros you're proposing to introduce only get used in > one place. > > 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 started grepping RELKIND_MATVIEW and convert corresponding conditions into macros. I have gone through all the instances yet, so I am not sure if all the macros are going to be used in only one place. I will come to know that only once I have gone through all such instances. Some of the macros have same relkinds involved e.g. RELKIND_HAS_VISIBILITY_MAP and RELKIND_HAS_XIDS or RELKIND_CAN_HAVE_TOAST_TABLE and RELKIND_CAN_HAVE_INDEX. In such cases it may be better to use a single macro instead of two by using a name indicating some common property behind those tests. Can we say that any relation that has visibility map will also have xids? If yes, what's that common property? Similarly can any relation that can have toast table also have an index? If yes, what's the common property? I didn't get time to investigate it further. -- 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] map_partition_varattnos() and whole-row vars
Thanks for the review.
On 2017/08/03 13:54, Amit Khandekar wrote:
> On 2 August 2017 at 11:51, Amit Langote wrote:
>> On 2017/08/02 1:33, Amit Khandekar wrote:
>>> Instead of callers of map_partition_varattnos() reporting error, we
>>> can have map_partition_varattnos() itself report error. Instead of the
>>> found_whole_row parameter of map_partition_varattnos(), we can have
>>> error_on_whole_row parameter. So callers who don't expect whole row,
>>> would pass error_on_whole_row=true to map_partition_varattnos(). This
>>> will simplify the resultant code a bit.
>>
>> So, I think it's only the callers of
>> map_partition_varattnos() who know whether finding whole-row vars is an
>> error and *what kind* of error.
>
> Ok. Got it. Thanks for the explanation.
>
> How about making found_whole_row as an optional parameter ?
> map_partition_varattnos() would populate it only if its not NULL. This
> way, callers who don't bother about whether it is found or not don't
> have to declare a variable and pass its address. In current code,
> ExecInitModifyTable() is the one who has to declare found_whole_row
> without needing it.
Sure, done. But while implementing that, I avoided changing anything in
map_variable_attnos(), such as adding a check for not-NULLness of
found_whole_row. The only check added is in map_partition_varattnos().
Attached updated patches.
Thanks,
Amit
From 00a46ef3cfac3b3b197ccf39b8749f06d0d942ad 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 | 17 +++--
src/backend/commands/tablecmds.c | 8 +++-
src/backend/executor/nodeModifyTable.c| 14 ++
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 | 6 ++
src/test/regress/sql/updatable_views.sql | 9 +
8 files changed, 65 insertions(+), 12 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 9d50efb6a0..5891af9cf2 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -904,10 +904,11 @@ get_qual_from_partbound(Relation rel, Relation parent,
*/
List *
map_partition_varattnos(List *expr, int target_varno,
- Relation partrel, Relation
parent)
+ Relation partrel, Relation
parent,
+ bool *found_whole_row)
{
AttrNumber *part_attnos;
- boolfound_whole_row;
+ boolmy_found_whole_row;
if (expr == NIL)
return NIL;
@@ -919,10 +920,9 @@ map_partition_varattnos(List *expr, int target_varno,
target_varno, 0,
part_attnos,
RelationGetDescr(parent)->natts,
-
&found_whole_row);
- /* There can never be a whole-row reference here */
+
&my_found_whole_row);
if (found_whole_row)
- elog(ERROR, "unexpected whole-row reference found in partition
key");
+ *found_whole_row = my_found_whole_row;
return expr;
}
@@ -1783,6 +1783,7 @@ generate_partition_qual(Relation rel)
List *my_qual = NIL,
*result = NIL;
Relationparent;
+ boolfound_whole_row;
/* Guard against stack overflow due to overly deep partition tree */
check_stack_depth();
@@ -1825,7 +1826,11 @@ generate_partition_qual(Relation rel)
* in it to bear this relation's attnos. It's safe to assume varno = 1
* here.
*/
- result = map_partition_varattnos(result, 1, rel, parent);
+ result = map_partition_varattnos(result, 1, rel, parent,
+
Re: [HACKERS] Unused variable scanned_tuples in LVRelStats
On Wed, Aug 2, 2017 at 11:40 PM, Robert Haas wrote: > On Tue, Jul 4, 2017 at 10:13 PM, Masahiko Sawada > wrote: >> scanned_tuples variable in LVRelStats is introduced by commit b4b6923e >> but it seems to me that it's actually not used. We store num_tuples >> into vacrelstats->scanned_tuples after scanned all blocks, and the >> comment mentioned that saving it in order to use later but we actually >> use num_tuples instead of vacrelstats->scanned_tuples from there. I >> think the since the name of scanned_tuples implies more clearer >> purpose than num_tuples it's better to use it instead of num_tuples, >> or we can remove scanned_tuples from LVRelStats. Thank you for the comment! > > I think we should only store stuff in LVRelStats if it needs to be > passed to some other function. Agreed. From this point of view, num_tuples is only one variable of LVRelStats that is not passed to other functions. > Data that's only used in > lazy_scan_heap() can just be kept in local variables. We could rename > the local variable, though, since I agree with you that scanned_tuples > is clearer. > So we can remove scanned_tuples from LVRelStats struct and change the variable name num_tuples to scanned_tuples. Attached updated patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_vacuumlazy_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()
On 2017/08/02 20:35, Robert Haas wrote:
> On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote
> wrote:
>> I too dislike the shape of attachRel. How about we rename attachRel to
>> attachrel? So, attachrel_children, attachrel_constr, etc. It's still
>> long though... :)
>
> OK, I can live with that, I guess.
Alright, attached updated 0001 does that.
About the other hunk, it seems we will have to go with the following
structure after all;
if (a)
if (b)
c();
d();
Note that we were missing that there is a d(), which executes if a is
true. c() executes *only* if b is true. So I left that hunk unchanged,
viz. the following:
/*
- * Skip if it's a partitioned table. Only RELKIND_RELATION
- * relations (ie, leaf partitions) need to be scanned.
+ * Skip if the partition is itself a partitioned table. We can
+ * only ever scan RELKIND_RELATION relations.
*/
-if (part_rel != attachRel &&
-part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
-heap_close(part_rel, NoLock);
+if (part_rel != attachrel)
+heap_close(part_rel, NoLock);
continue;
}
You might ask why the earlier code worked if there was this kind of
logical bug - accident; even if we failed skipping attachRel, the AT
rewrite phase which is in charge of actually scanning the table knows to
skip the partitioned tables, so no harm would be done.
Thanks,
Amit
From 4558c1b31f10e5446a22850a7f8b3d80d082330d Mon Sep 17 00:00:00 2001
From: amit
Date: Tue, 1 Aug 2017 10:12:39 +0900
Subject: [PATCH 1/4] Cosmetic fixes for code in ATExecAttachPartition
---
src/backend/commands/tablecmds.c | 125 +++
1 file changed, 61 insertions(+), 64 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..ecfc7e48c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13419,10 +13419,10 @@ ComputePartitionAttrs(Relation rel, List *partParams,
AttrNumber *partattrs,
static ObjectAddress
ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
{
- RelationattachRel,
+ Relationattachrel,
catalog;
- List *childrels;
- TupleConstr *attachRel_constr;
+ List *attachrel_children;
+ TupleConstr *attachrel_constr;
List *partConstraint,
*existConstraint;
SysScanDesc scan;
@@ -13434,22 +13434,22 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
ObjectAddress address;
const char *trigger_name;
- attachRel = heap_openrv(cmd->name, AccessExclusiveLock);
+ attachrel = heap_openrv(cmd->name, AccessExclusiveLock);
/*
* Must be owner of both parent and source table -- parent was checked
by
* ATSimplePermissions call in ATPrepCmd
*/
- ATSimplePermissions(attachRel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(attachrel, ATT_TABLE | ATT_FOREIGN_TABLE);
/* A partition can only have one parent */
- if (attachRel->rd_rel->relispartition)
+ if (attachrel->rd_rel->relispartition)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is already a partition",
-
RelationGetRelationName(attachRel;
+
RelationGetRelationName(attachrel;
- if (OidIsValid(attachRel->rd_rel->reloftype))
+ if (OidIsValid(attachrel->rd_rel->reloftype))
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot attach a typed table as
partition")));
@@ -13462,7 +13462,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
ScanKeyInit(&skey,
Anum_pg_inherits_inhrelid,
BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(RelationGetRelid(attachRel)));
+ ObjectIdGetDatum(RelationGetRelid(attachrel)));
scan = systable_beginscan(catalog, InheritsRelidSeqnoIndexId, true,
NULL, 1, &skey);
if (HeapTupleIsValid(systable_getnext(scan)))
@@ -13475,11 +13475,11 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
ScanKeyInit(&skey,
Anum_pg_inherits_inhparent,
BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatu
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2 August 2017 at 11:51, 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.
>
>> 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.
>
Attached 2 patches:
0001: Addresses the bug that Rajkumar reported (handling whole-row vars in
WITH CHECK and RETURNING expressions at all)
0002: Addressed the bug that Amit reported (converting whole-row vars
that could occur in WITH CHECK and RETURNING expressions)
>>>
>>>
>>> I took a quick look at the patches. One thing I noticed is this:
>>>
>>> map_variable_attnos(Node *node,
>>> int target_varno, int sublevels_up,
>>> const AttrNumber *attno_map, int
>>> map_length,
>>> + Oid from_rowtype, Oid to_rowtype,
>>> bool *found_whole_row)
>>> {
>>> map_variable_attnos_context context;
>>> @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node,
>>> context.sublevels_up = sublevels_up;
>>> context.attno_map = attno_map;
>>> context.map_length = map_length;
>>> + context.from_rowtype = from_rowtype;
>>> + context.to_rowtype = to_rowtype;
>>> context.found_whole_row = found_whole_row;
>>>
>>> You added two arguments to pass to map_variable_attnos(): from_rowtype and
>>> to_rowtype. But I'm not sure that we really need the from_rowtype argument
>>> because it's possible to get the Oid from the vartype of target whole-row
>>> Vars. And in this:
>>>
>>> + /*
>>> +* If the callers expects us to convert the
>>> same, do so if
>>> +* necessary.
>>> +*/
>>> + if (OidIsValid(context->to_rowtype) &&
>>> + OidIsValid(context->from_rowtype) &&
>>> + context->to_rowtype !=
>>> context->from_rowtype)
>>> + {
>>> + ConvertRowtypeExpr *r =
>>> makeNode(ConvertRowtypeExpr);
>>> +
>>> + r->arg = (Expr *) newvar;
>>> + r->resulttype =
>>> context->from_rowtype;
>>> + r->convertformat =
>>> COERCE_IMPLICIT_CAST;
>>> + r->location = -1;
>>> + /* Make sure the Var node has the
>>> right type ID, too */
>>> + newvar->vartype =
>>> context->to_rowtype;
>>> + return (Node *) r;
>>> + }
>>>
>>> I think we could set r->resulttype to the vartype (ie, "r->resulttype =
>>> newvar->vartype" instead of "r->resulttype = context->from_rowtype").
>>
>> I agree.
>
> You're right, from_rowtype is unnecessary.
>
>> ---
>>
>> 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.
>
>> ---
>>
>> - result = map_partition_varattnos(result, 1, rel, parent);
>> + result = map_partition_varattnos(result, 1, rel, parent,
>> +
>> &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");
>>
>> Instead of callers of map_partition_varattnos() reporting error, we
>> can have map_partition_varattnos() itself report
Re: [HACKERS] foreign table creation and NOT VALID check constraints
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.) 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] INSERT ON CONFLICT and partitioned tables
Thanks Jeevan for looking at this. See comments below. On 2017/08/02 19:04, Jeevan Ladhe wrote: > On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote wrote: >> The patch's job is simple: >> >> - Remove the check in the parser that causes an error the moment the >> ON CONFLICT clause is found. >> >> - Fix leaf partition ResultRelInfo initialization code so that the call >> ExecOpenIndices() specifies 'true' for speculative, so that the >> information necessary for conflict checking will be initialized in the >> leaf partition's ResultRelInfo > > I applied the patch on latest master sources and the patch applies cleanly. > The documentation is built without errors. > > We do not support following syntax for 'do nothing': > > postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b) > do nothing; > ERROR: there is no unique or exclusion constraint matching the ON CONFLICT > specification To nitpick, the above is not a syntax error; we *do* support the syntax to specify conflict target even when the conflict action is DO NOTHING. The error is emitted by the planner when it fails to find the index to cover column 'b' that's specified as the conflict target. > This limitation is because we do not support unique index on partitioned > table. Yes. > But, in that sense the following snippet of the documentation seems > misleading: > > + will cause an error if the conflict target is specified (see > +for more details). That means it's not > + possible to specify DO UPDATE as the alternative > + action, because it requires the conflict target to be specified. > + On the other hand, specifying DO NOTHING as the > + alternative action works fine. > May be the last sentence can be rephrased as below: > > "On the other hand, specifying DO NOTHING without target > as > an alternative action works fine." I have updated the text. > Other than this patch looks good to me. Updated patch attached. Thanks, Amit [1] https://www.postgresql.org/docs/devel/static/sql-insert.html#sql-on-conflict From ec32e99310c034b26db1c5478ed38641150a7aec Mon Sep 17 00:00:00 2001 From: amit Date: Mon, 3 Apr 2017 19:13:38 +0900 Subject: [PATCH] Allow ON CONFLICT DO NOTHING on partitioned tables ON CONFLICT .. DO UPDATE still doesn't work, because it requires specifying the conflict target. DO NOTHING doesn't require it, but the executor will check for conflicts within only a given leaf partitions, if relevant constraints exist. Specifying the conflict target makes the planner look for the required indexes on the parent table, which are not allowed, so an error will always be reported in that case. --- doc/src/sgml/ddl.sgml | 12 +--- src/backend/executor/execMain.c | 10 ++ src/backend/parser/analyze.c | 8 src/test/regress/expected/insert_conflict.out | 10 ++ src/test/regress/sql/insert_conflict.sql | 10 ++ 5 files changed, 35 insertions(+), 15 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index b05a9c2150..23b0c42dba 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -3276,9 +3276,15 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 Using the ON CONFLICT clause with partitioned tables - will cause an error, because unique or exclusion constraints can only be - created on individual partitions. There is no support for enforcing - uniqueness (or an exclusion constraint) across an entire partitioning + will cause an error if the conflict target is specified (see +for more details on how the clause + works). That means it's not possible to specify + DO UPDATE as the alternative action, because + specifying the conflict target is mandatory in that case. On the other + hand, specifying DO NOTHING as the alternative action + works fine provided the conflict target is not specified. In that case, + unique constraints (or exclusion constraints) of the individual leaf + partitions are considered, not those across the whole partitioning hierarchy. diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index c11aa4fe21..b313ad1efa 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -3300,13 +3300,15 @@ ExecSetupPartitionTupleRouting(Relation rel, 0); /* -* Open partition indices (remember we do not support ON CONFLICT in -* case of partitioned tables, so we do not need support information -* for speculative insertion) +* Open partition indices. The user may have asked to check for +* conflicts within this leaf partition and do "nothing" instead of +* throwing an error. Be prepare
Re: [HACKERS] fixing pg_upgrade strings (was Re: pgsql: Add new files to nls.mk and add translation)
Alvaro Herrera writes:
> It does this:
> pg_fatal("You must identify the directory where the %s.\n"
> "Please use the %s command-line option or the %s
> environment variable.\n",
> description, cmdLineOption, envVarName);
> and the callsites are like this:
> check_required_directory(&new_cluster.bindir, NULL, "PGBINNEW", "-B",
> _("new cluster binaries reside"));
> check_required_directory(&old_cluster.pgdata, &old_cluster.pgconfig,
> "PGDATAOLD", "-d", _("old cluster data
> resides"));
> note the declensions don't match even in the English original.
FWIW, I think the English declensions are fine as long as you consider
"data" to be a group noun --- "data reside" would read a bit funny IMO.
But certainly this is an utter translatability fail. Maybe pass a boolean
to indicate one of two first sentences to use? Or actually, might as well
consider the entire message string to be one of two translatable options.
It's not like we have a need to indicate more than one option or envar
name for each case.
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] [COMMITTERS] pgsql: Fix pg_dump's errno checking for zlib I/O
Alvaro Herrera writes: > Alvaro Herrera wrote: >> Fix pg_dump's errno checking for zlib I/O > So this broke a few buildfarm members. I'll into it tomorrow. I think you forgot to consider the !HAVE_LIBZ case. 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] intermittent failures in Cygwin from select_parallel tests
On Wed, Jun 21, 2017 at 06:44:09PM -0400, Tom Lane wrote:
> Today, lorikeet failed with a new variant on the bgworker start crash:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2017-06-21%2020%3A29%3A10
>
> This one is even more exciting than the last one, because it sure looks
> like the crashing bgworker took the postmaster down with it. That is
> Not Supposed To Happen.
>
> Wondering if we broke something here recently, I tried to reproduce it
> on a Linux machine by adding a randomized Assert failure in
> shm_mq_set_sender. I don't see any such problem, even with EXEC_BACKEND;
> we recover from the crash as-expected.
>
> So I'm starting to get a distinct feeling that there's something wrong
> with the cygwin port. But I dunno what.
I think signal blocking broke on Cygwin.
On a system (gcc 5.4.0, CYGWIN_NT-10.0 2.7.0(0.306/5/3) 2017-02-12 13:18
x86_64) that reproduces lorikeet's symptoms, I instrumented the postmaster as
attached. The patch's small_parallel.sql is a subset of select_parallel.sql
sufficient to reproduce the mq_sender Assert failure and the postmaster silent
exit. (It occasionally needed hundreds of iterations to do so.) The parallel
query normally starts four bgworkers; when the mq_sender Assert fired, the
test had started five workers in response to four registrations.
The postmaster.c instrumentation regularly detects sigusr1_handler() calls
while another sigusr1_handler() is already on the stack:
6328 2017-08-02 07:25:42.788 GMT LOG: forbid signals @ sigusr1_handler
6328 2017-08-02 07:25:42.788 GMT DEBUG: saw slot-0 registration, want 0
6328 2017-08-02 07:25:42.788 GMT DEBUG: saw slot-0 registration, want 1
6328 2017-08-02 07:25:42.788 GMT DEBUG: slot 1 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG: registering background worker
"parallel worker for PID 4776" (slot 1)
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-1 registration, want 2
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-0 registration, want 2
6328 2017-08-02 07:25:42.789 GMT DEBUG: slot 2 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG: registering background worker
"parallel worker for PID 4776" (slot 2)
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-2 registration, want 3
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-1 registration, want 3
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-0 registration, want 3
6328 2017-08-02 07:25:42.789 GMT DEBUG: slot 3 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG: registering background worker
"parallel worker for PID 4776" (slot 3)
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-3 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-2 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-1 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG: saw slot-0 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG: slot 4 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG: registering background worker
"parallel worker for PID 4776" (slot 4)
6328 2017-08-02 07:25:42.789 GMT DEBUG: starting background worker process
"parallel worker for PID 4776"
6328 2017-08-02 07:25:42.790 GMT LOG: forbid signals @ sigusr1_handler
6328 2017-08-02 07:25:42.790 GMT WARNING: signals already forbidden @
sigusr1_handler
6328 2017-08-02 07:25:42.790 GMT LOG: permit signals @ sigusr1_handler
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.
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index 17b1038..f42034d 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -985,6 +985,8 @@ ParallelWorkerMain(Datum main_arg)
error_queue_space = shm_toc_lookup(toc, PARALLEL_KEY_ERROR_QUEUE,
false);
mq = (shm_mq *) (error_queue_space +
ParallelWorkerNumber *
PARALLEL_ERROR_QUEUE_SIZE);
+ write_stderr("PID %d claiming queue %d (%p)\n",
+MyProc->pid, ParallelWorkerNumber, mq);
shm_mq_set_sender(mq, MyProc);
mqh = shm_mq_attach
[HACKERS] fixing pg_upgrade strings (was Re: pgsql: Add new files to nls.mk and add translation)
Peter Eisentraut wrote:
> Add new files to nls.mk and add translation markers
This reminds me that I noticed a few days ago another really serious
broken piece in pg_upgrade where check_required_directory() is incurring
in the ugliest case of string building I've ever seen. I didn't have
the courage to try to fix it back then, but I think we should dedicate
it some time. It does this:
pg_fatal("You must identify the directory where the %s.\n"
"Please use the %s command-line option or the %s
environment variable.\n",
description, cmdLineOption, envVarName);
and the callsites are like this:
check_required_directory(&new_cluster.bindir, NULL, "PGBINNEW", "-B",
_("new cluster binaries reside"));
check_required_directory(&old_cluster.pgdata, &old_cluster.pgconfig,
"PGDATAOLD", "-d", _("old cluster data resides"));
note the declensions don't match even in the English original.
--
Á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] map_partition_varattnos() and whole-row vars
On 2017/08/03 12:06, Robert Haas wrote: On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote wrote: On 2017/08/02 15:21, Amit Langote wrote: Thanks Fuita-san and Amit for reviewing. Sorry, I meant Fujita-san. Time is growing short here. Is there more review or coding that needs to be done here? I'll take another look at the patch from now. 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] foreign table creation and NOT VALID check constraints
On Wed, Aug 2, 2017 at 9:41 PM, Amit Langote wrote: > On 2017/08/02 20:40, Robert Haas wrote: >> On Wed, Aug 2, 2017 at 3:46 AM, Ashutosh Bapat >> wrote: >>> If the user has specified "not valid" for a constraint on the foreign >>> table, there is high chance that s/he is aware of the fact that the >>> remote table that the foreign table points to has some rows which will >>> violet the constraint. So, +1. >> >> +1 from me, too. > > Alright, thanks. > > 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. -- 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] map_partition_varattnos() and whole-row vars
On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote wrote: > On 2017/08/02 15:21, Amit Langote wrote: >> Thanks Fuita-san and Amit for reviewing. > > Sorry, I meant Fujita-san. Time is growing short here. Is there more review or coding that needs to be done here? -- 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] Another comment typo in execMain.c
On 2017/08/01 6:09, Peter Eisentraut wrote: On 7/6/17 03:23, Etsuro Fujita wrote: Here is a comment in ExecFindPartition() in execMain.c: /* * First check the root table's partition constraint, if any. No point in * routing the tuple it if it doesn't belong in the root table itself. */ I think that in the second sentence "it" just before "if" is a typo. Attached is a small patch for fixing that. fixed 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] pg_stop_backup(wait_for_archive := true) on standby server
Noah, On Tue, Aug 1, 2017 at 20:52 Noah Misch wrote: > On Thu, Jul 27, 2017 at 10:27:36AM -0400, Stephen Frost wrote: > > Noah, all, > > > > * Noah Misch (n...@leadboat.com) wrote: > > > This PostgreSQL 10 open item is past due for your status update. > Kindly send > > > a status update within 24 hours, and include a date for your > subsequent status > > > update. Refer to the policy on open item ownership: > > > > Based on the ongoing discussion, this is really looking like it's > > actually a fix that needs to be back-patched to 9.6 rather than a PG10 > > open item. I don't have any issue with keeping it as an open item > > though, just mentioning it. I'll provide another status update on or > > before Monday, July 31st. > > This PostgreSQL 10 open item is past due for your status update. Kindly > send > a status update within 24 hours, and include a date for your subsequent > status > update. I'll provide another update tomorrow. Hopefully Michael is able to produce a 9.6 patch, otherwise I'll do it. Thanks! Stephen >
Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_dump's errno checking for zlib I/O
Alvaro Herrera wrote: > Fix pg_dump's errno checking for zlib I/O So this broke a few buildfarm members. I'll into it tomorrow. -- Á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] foreign table creation and NOT VALID check constraints
On 2017/08/02 20:40, Robert Haas wrote:
> On Wed, Aug 2, 2017 at 3:46 AM, Ashutosh Bapat
> wrote:
>> If the user has specified "not valid" for a constraint on the foreign
>> table, there is high chance that s/he is aware of the fact that the
>> remote table that the foreign table points to has some rows which will
>> violet the constraint. So, +1.
>
> +1 from me, too.
Alright, thanks.
Attached is a patch. I think this could be considered a bug-fix,
backpatchable to 9.6 which introduced this behavior change [1].
Thoughts?
Thanks,
Amit
[1]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f27a6b15e656
From a0967f1a71a65e7802f504002a6dc3dfd1f4a6bb Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 3 Aug 2017 10:31:21 +0900
Subject: [PATCH] Honor NOT VALID specification in CREATE FOREIGN TABLE
It should be possible for a user to mark the constraints on foreign
tables as NOT VALID even when creating the table, because the remote
data they point to may contain constraint-violating rows, which the
database has no way to detect and show an error message for.
---
doc/src/sgml/ref/create_foreign_table.sgml | 7 +++
doc/src/sgml/ref/create_table.sgml | 7 +++
src/backend/parser/parse_utilcmd.c | 3 ++-
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/create_foreign_table.sgml
b/doc/src/sgml/ref/create_foreign_table.sgml
index 065c982082..72bf37b8b9 100644
--- a/doc/src/sgml/ref/create_foreign_table.sgml
+++ b/doc/src/sgml/ref/create_foreign_table.sgml
@@ -222,6 +222,13 @@ CHECK ( expression ) [ NO INHERIT ]
A constraint marked with NO INHERIT will not propagate to
child tables.
+
+
+ It is possible to mark the constraint as NOT VALID if it is
+ specified as a table constraint. If marked as such, the database will
+ not assume that the constraint holds for all the rows in the table until
+ it is validated by using the VALIDATE CONSTRAINT command.
+ See .
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index e9c2c49533..72de64a03e 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -652,6 +652,13 @@ FROM ( { numeric_literal | PostgreSQL versions before 9.5 did not honor any
particular firing order for CHECK constraints.)
+
+
+ Note that even if the constraint is marked as NOT VALID,
+ it is considered validated as the table that is just created cannot
+ contain any data. In other words, specifying NOT VALID in
+ this case has no effect.
+
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index 9f37f1b920..e54322b460 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -165,6 +165,7 @@ transformCreateStmt(CreateStmt *stmt, const char
*queryString)
Oid existing_relid;
ParseCallbackState pcbstate;
boollike_found = false;
+ boolis_foreign_table = IsA(stmt, CreateForeignTableStmt);
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This
is
@@ -330,7 +331,7 @@ transformCreateStmt(CreateStmt *stmt, const char
*queryString)
/*
* Postprocess check constraints.
*/
- transformCheckConstraints(&cxt, true);
+ transformCheckConstraints(&cxt, !is_foreign_table ? true : false);
/*
* Output results.
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Why does logical replication launcher exit with exit code 1?
On Wed, Aug 2, 2017 at 9:33 PM, Peter Eisentraut wrote: > On 8/2/17 16:52, Robert Haas wrote: >> I actually don't think it's that unreasonable to get notified when >> system-wide processes like the autovacuum launcher or the logical >> replication launcher start or stop. > > But we got rid of those start messages recently due to complaints. Yeah, true. I'm just talking about what *I* think. :-) -- 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] Why does logical replication launcher exit with exit code 1?
On 8/2/17 16:52, Robert Haas wrote: > I actually don't think it's that unreasonable to get notified when > system-wide processes like the autovacuum launcher or the logical > replication launcher start or stop. But we got rid of those start messages recently due to complaints. -- 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] Partition-wise join for join between (declaratively) partitioned tables
On Mon, Jul 31, 2017 at 9:07 AM, Ashutosh Bapat wrote: > Forgot the patch set. Here it is. The commit message for 0005 isn't really accurate given that it follows 0004. I think you could just flatten 0005 and 0006 into one patch. Reviewing those together: - Existing code does partdesc = RelationGetPartitionDesc(relation) but this has got it as part_desc. Seems better to be consistent. Likewise existing variables for PartitionKey are key or partkey, not part_key. - get_relation_partition_info has a useless trailing return. - Instead of adding nparts, boundinfo, and part_oids to RelOptInfo, how about just adding partdesc? Seems cleaner. - pkexprs seems like a potentially confusing name, since PK is widely used to mean "primary key" but here you mean "partition key". Maybe partkeyexprs. - build_simple_rel's matching algorithm is O(n^2). We may have talked about this problem before... - This patch introduces some bits that are not yet used, like nullable_pkexprs, or even the code to set the partition scheme for joinrels. I think perhaps some of that logic should be moved from 0008 to here - e.g. the initial portion of build_joinrel_partition_info. There may be more, but I've run out of energy for tonight. -- 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] Confusing error message in pgbench
>> Not really objecting, but an even better fix might be to remove the >> restriction on the order in which the options can be specified. > > 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. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] asynchronous execution
Thank you for the comment. At Tue, 1 Aug 2017 16:27:41 -0400, Robert Haas wrote in > On Mon, Jul 31, 2017 at 5:42 AM, Kyotaro HORIGUCHI > wrote: > > Another is getting rid of recursive call to run an execution > > tree. > > That happens to be exactly what Andres did for expression evaluation > in commit b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755, and I think > generalizing that to include the plan tree as well as expression trees > is likely to be the long-term way forward here. I read it in the source tree. The patch implements converting expression tree to an intermediate expression then run it on a custom-made interpreter. Guessing from the word "upside down" from Andres, the whole thing will become source-driven. > Unfortunately, that's probably another gigantic patch (that > should probably be written by Andres). Yeah, but async executor on the current style of executor seems furtile work, or sitting until the patch comes is also waste of time. So I'm planning to include the following sutff in the next PoC patch. Even I'm not sure it can land on the coming Andres'patch. - Tuple passing outside call-stack. (I remember it was in the past of the thread around but not found) This should be included in the Andres' patch. - Give executor an ability to run from data-source (or driver) nodes to the root. I'm not sure this is included, but I suppose he is aiming this kind of thing. - Rebuid asynchronous execution on the upside-down executor. regrds, -- Kyotaro Horiguchi 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] Confusing error message in pgbench
> Not really objecting, but an even better fix might be to remove the > restriction on the order in which the options can be specified. +100 :-) Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
On 2017-02-10 07:52:57 -0500, Robert Haas wrote: > On Thu, Feb 9, 2017 at 6:38 PM, Thomas Munro > > Up until two minutes ago I assumed that policy would leave only two > > possibilities: you attach to the DSM segment and attach to the > > SharedBufFileManager successfully or you attach to the DSM segment and > > then die horribly (but not throw) and the postmaster restarts the > > whole cluster and blows all temp files away with RemovePgTempFiles(). > > But I see now in the comment of that function that crash-induced > > restarts don't call that because "someone might want to examine the > > temp files for debugging purposes". Given that policy for regular > > private BufFiles, I don't see why that shouldn't apply equally to > > shared files: after a crash restart, you may have some junk files that > > won't be cleaned up until your next clean restart, whether they were > > private or shared BufFiles. > > I think most people (other than Tom) would agree that that policy > isn't really sensible any more; it probably made sense when the > PostgreSQL user community was much smaller and consisted mostly of the > people developing PostgreSQL, but these days it's much more likely to > cause operational headaches than to help a developer debug. FWIW, we have restart_after_crash = false. If you need to debug things, you can enable that. Hence the whole RemovePgTempFiles() crash-restart exemption isn't required anymore, we have a much more targeted solution. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Kunshchikov Vladimir wrote: > Hello Alvaro, > > here goes v4 version: removed unused header. > > Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce > any warnings. Great, thanks! I have pushed this to all branches since 9.4. Would you please give it a look? Please let me know if you find any problems (particularly since you seem to have a test rig to verify it on corrupted files). I noticed that with this patch we no longer use WRITE_ERROR_EXIT in certain cases but instead do the printing/exiting directly. I think that's fine, but it would be neater to improve the WRITE_ERROR_EXIT macro so that it takes the cfp as an argument, and then the macro is in charge of calling get_cfp_error. But then I noticed that it wasn't very easy to improve things that way. I also noticed that the usage of mixed compressed/uncompressed file pointers in pg_dump is not very consistent, and it would take rather a lot of effort to clean up. So I gave up for now, particularly as a backpatchable bugfix. If you're interested in mop-up work, I think we can improve pg_dump some more there. At least, I think most common cases should correctly report any zlib problems now. Thanks for the patch! -- Á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] Why does logical replication launcher exit with exit code 1?
On 2017-08-02 17:09:10 -0400, Tom Lane wrote: > Even if we fix that, though, I think it is reasonable to downgrade it to > DEBUG1. We did that already for other standard background processes such > as the autovac launcher, and it's not apparent to me why the logrep > launcher should be chattier. Now, *unexpected* starts or stops should > certainly deserve a higher log rating ... but the general rule ought to be > that totally-expected behavior does not deserve a log entry by default. Well, every exit *other* than during a shutdown is unexpected. That's why I suggested changing the log level for exit code 1 depending on the cluster being shut down or not. - 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] Why does logical replication launcher exit with exit code 1?
Andres Freund writes: > On 2017-08-02 16:52:01 -0400, Robert Haas wrote: >> I actually don't think it's that unreasonable to get notified when >> system-wide processes like the autovacuum launcher or the logical >> replication launcher start or stop. That's stuff somebody might want >> to know. It's not going to generate a lot of log volume, and it might >> be useful, so why suppress it? > I generally agree. But in the shutdown case it's just useless and > confusing - the launcher is stopping because the entire server is being > stopped, and that's very much not clear from the message. Yes. The main complaint here is not about the existence of the message but about the fact that it looks like it's reporting a failure. I would vote for removing it if it's not simple to fix that problem. Even if we fix that, though, I think it is reasonable to downgrade it to DEBUG1. We did that already for other standard background processes such as the autovac launcher, and it's not apparent to me why the logrep launcher should be chattier. Now, *unexpected* starts or stops should certainly deserve a higher log rating ... but the general rule ought to be that totally-expected behavior does not deserve a log entry by default. 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] Why does logical replication launcher exit with exit code 1?
On 2017-08-02 16:52:01 -0400, Robert Haas wrote: > On Wed, Aug 2, 2017 at 4:38 PM, Peter Eisentraut > wrote: > > Maybe it doesn't need to be logged at all (other than perhaps as DEBUG)? > > A few months ago, people were complaining about too many messages about > > background workers starting. Now we are having complaints about > > messages about background workers stopping. > > I actually don't think it's that unreasonable to get notified when > system-wide processes like the autovacuum launcher or the logical > replication launcher start or stop. That's stuff somebody might want > to know. It's not going to generate a lot of log volume, and it might > be useful, so why suppress it? > > Where things get ugly is if you start to get a high rate of messages - > e.g. from starting and stopping parallel query workers or other kinds > of things where you might have workers starting and stopping very > frequently. But surely this isn't an example of that. I generally agree. But in the shutdown case it's just useless and confusing - the launcher is stopping because the entire server is being stopped, and that's very much not clear from the message. - 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] Why does logical replication launcher exit with exit code 1?
On Wed, Aug 2, 2017 at 4:38 PM, Peter Eisentraut wrote: > Maybe it doesn't need to be logged at all (other than perhaps as DEBUG)? > A few months ago, people were complaining about too many messages about > background workers starting. Now we are having complaints about > messages about background workers stopping. I actually don't think it's that unreasonable to get notified when system-wide processes like the autovacuum launcher or the logical replication launcher start or stop. That's stuff somebody might want to know. It's not going to generate a lot of log volume, and it might be useful, so why suppress it? Where things get ugly is if you start to get a high rate of messages - e.g. from starting and stopping parallel query workers or other kinds of things where you might have workers starting and stopping very frequently. But surely this isn't an example of that. -- 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] POC: Cache data in GetSnapshotData()
Hi,
I think this patch should have a "cover letter" explaining what it's
trying to achieve, how it does so and why it's safe/correct. I think
it'd also be good to try to show some of the worst cases of this patch
(i.e. where the caching only adds overhead, but never gets used), and
some of the best cases (where the cache gets used quite often, and
reduces contention significantly).
I wonder if there's a way to avoid copying the snapshot into the cache
in situations where we're barely ever going to need it. But right now
the only way I can think of is to look at the length of the
ProcArrayLock wait queue and count the exclusive locks therein - which'd
be expensive and intrusive...
On 2017-08-02 15:56:21 +0530, Mithun Cy wrote:
> diff --git a/src/backend/storage/ipc/procarray.c
> b/src/backend/storage/ipc/procarray.c
> index a7e8cf2..4d7debe 100644
> --- a/src/backend/storage/ipc/procarray.c
> +++ b/src/backend/storage/ipc/procarray.c
> @@ -366,11 +366,13 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
> (arrayP->numProcs - index - 1) *
> sizeof(int));
> arrayP->pgprocnos[arrayP->numProcs - 1] = -1; /* for
> debugging */
> arrayP->numProcs--;
> + ProcGlobal->is_snapshot_valid = false;
> LWLockRelease(ProcArrayLock);
> return;
> }
> }
>
> + ProcGlobal->is_snapshot_valid = false;
> /* Oops */
> LWLockRelease(ProcArrayLock);
Why do we need to do anything here? And if we need to, why not in
ProcArrayAdd?
> @@ -1552,6 +1557,8 @@ GetSnapshotData(Snapshot snapshot)
>errmsg("out of memory")));
> }
>
> + snapshot->takenDuringRecovery = RecoveryInProgress();
> +
> /*
>* It is sufficient to get shared lock on ProcArrayLock, even if we are
>* going to set MyPgXact->xmin.
> @@ -1566,100 +1573,200 @@ GetSnapshotData(Snapshot snapshot)
> /* initialize xmin calculation with xmax */
> globalxmin = xmin = xmax;
>
> - snapshot->takenDuringRecovery = RecoveryInProgress();
> -
This movement of code seems fairly unrelated?
> if (!snapshot->takenDuringRecovery)
> {
Do we really need to restrict this to !recovery snapshots? It'd be nicer
if we could generalize it - I don't immediately see anything !recovery
specific in the logic here.
> - int*pgprocnos = arrayP->pgprocnos;
> - int numProcs;
> + if (ProcGlobal->is_snapshot_valid)
> + {
> + Snapshotcsnap = &ProcGlobal->cached_snapshot;
> + TransactionId *saved_xip;
> + TransactionId *saved_subxip;
>
> - /*
> - * Spin over procArray checking xid, xmin, and subxids. The
> goal is
> - * to gather all active xids, find the lowest xmin, and try to
> record
> - * subxids.
> - */
> - numProcs = arrayP->numProcs;
> - for (index = 0; index < numProcs; index++)
> + saved_xip = snapshot->xip;
> + saved_subxip = snapshot->subxip;
> +
> + memcpy(snapshot, csnap, sizeof(SnapshotData));
> +
> + snapshot->xip = saved_xip;
> + snapshot->subxip = saved_subxip;
> +
> + memcpy(snapshot->xip, csnap->xip,
> +sizeof(TransactionId) * csnap->xcnt);
> + memcpy(snapshot->subxip, csnap->subxip,
> +sizeof(TransactionId) * csnap->subxcnt);
> + globalxmin = ProcGlobal->cached_snapshot_globalxmin;
> + xmin = csnap->xmin;
> +
> + count = csnap->xcnt;
> + subcount = csnap->subxcnt;
> + suboverflowed = csnap->suboverflowed;
> +
> + Assert(TransactionIdIsValid(globalxmin));
> + Assert(TransactionIdIsValid(xmin));
> + }
Can we move this to a separate function? In fact, could you check how
much effort it'd be to split cached, !recovery, recovery cases into
three static inline helper functions? This is starting to be hard to
read, the indentation added in this patch doesn't help... Consider doing
recovery, !recovery cases in a preliminary separate patch.
> + * Let only one of the caller cache the computed
> snapshot, and
> + * others can continue as before.
>*/
> - if (!suboverflowed)
> + if (!ProcGlobal->is_snapshot_valid &&
> +
> LWLockConditionalAcquire(&ProcGlobal->CachedSnapshotLock,
> +
> LW_EXCLUSIVE))
Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables
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?
/*
* 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).
> 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."
It looks like you also need to update the header comment for
AppendRelInfo itself, in nodes/relation.h.
+ * PlannerInfo for every child is obtained by translating relevant members
Insert "The" at the start of the sentence.
-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.
+ * 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 PlannerInfo set up to be used while
+ * planning its children.
Most of this comment is in the singular, but the first half of the
second sentence is plural. Should be "Since a partitioned table does
not have any data...". I might replace the last sentence by "We do,
however, need to remember the PlannerInfo for use when planning its
children."
+-- Check UPDATE with *multi-level partitioned* inherited target
Asterisks seem like overkill.
Since expand_inherited_rtentry() and set_append_rel_size() can now
recurse down to as many levels as there are levels in the inheritance
hierarchy, they should probably have a check_stack_depth() check.
>> Overall I think this is a reasonable direction to go but I'm worried
>> that there may be bugs lurking -- other code that needs adjusting that
>> hasn't been found, really.
>>
> Planner code is already aware of such hierarchies except DMLs, which
> this patch adjusts. We have fixed issues revealed by mine and
> Rajkumar's testing.
> What kinds of things you suspect?
I'm not sure exactly. It's just hard with this kind of patch to make
sure you've caught everything.
--
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] Why does logical replication launcher exit with exit code 1?
On 8/1/17 20:20, Andres Freund wrote: > Well, that's how it is for all bgworkers - maybe a better solution is to > adjust that message in the postmaster rather than fiddle with the worker > exist code? Seems like we could easily take pmStatus into account > inside LogChildExit() and set the log level to DEBUG1 even for > EXIT_STATUS_1 in that case? Additionally we probably should always log > a better message for bgworkers exiting with exit 1, something about > unregistering the worker or such. Maybe it doesn't need to be logged at all (other than perhaps as DEBUG)? A few months ago, people were complaining about too many messages about background workers starting. Now we are having complaints about messages about background workers stopping. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.
On Tue, Jul 25, 2017 at 9:54 PM, Kyotaro HORIGUCHI wrote: > The patch is a kind of straightforward and looks fine for me. +1 for this change. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Failover Slots
On Tue, Jul 25, 2017 at 8:44 PM, Craig Ringer wrote: > No. The whole approach seems to have been bounced from core. I don't agree > and continue to think this functionality is desirable but I don't get to > make that call. I actually think failover slots are quite desirable, especially now that we've got logical replication in core. In a review of this thread I don't see anyone saying otherwise. The debate has really been about the right way of implementing that. Suppose we did something like this: - When a standby connects to a master, it can optionally supply a list of slot names that it cares about. - The master responds by periodically notifying the standby of changes to the slot contents using some new replication sub-protocol message. - The standby applies those updates to its local copies of the slots. So, you could create a slot on a standby with an "uplink this" flag of some kind, and it would then try to keep it up to date using the method described above. It's not quite clear to me how to handle the case where the corresponding slot doesn't exist on the master, or initially does but then it's later dropped, or it initially doesn't but it's later created. 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] Subscription code improvements
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Alvaro Herrera writes: > > I wish you'd stop splitting error message strings across multiple lines. > > I've been trapped by a faulty grep not matching a split error message a > > number of times :-( I know by now to remove words until I get a match, > > but it seems an unnecessary trap for the unwary. > > Yeah, that's my number one reason for not splitting error messages, too. > It's particularly nasty if similar strings appear in multiple places and > they're not all split alike, as you can get misled into thinking that a > reported error must have occurred in a place you found, rather than > someplace you didn't. +1. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Subscription code improvements
Alvaro Herrera writes: > I wish you'd stop splitting error message strings across multiple lines. > I've been trapped by a faulty grep not matching a split error message a > number of times :-( I know by now to remove words until I get a match, > but it seems an unnecessary trap for the unwary. Yeah, that's my number one reason for not splitting error messages, too. It's particularly nasty if similar strings appear in multiple places and they're not all split alike, as you can get misled into thinking that a reported error must have occurred in a place you found, rather than someplace you didn't. 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] Subscription code improvements
Petr Jelinek wrote: > I split it into several patches: I wish you'd stop splitting error message strings across multiple lines. I've been trapped by a faulty grep not matching a split error message a number of times :-( I know by now to remove words until I get a match, but it seems an unnecessary trap for the unwary. -- Á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] Subscription code improvements
On 8/1/17 00:17, Noah Misch wrote: > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. I'm looking into this now and will report by Friday. -- 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] reload-through-the-top-parent switch the partition table
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". -- 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] Confusing error message in pgbench
Robert Haas writes:
> On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii wrote:
>> I found an error message in pgbench is quite confusing.
> Not really objecting, but an even better fix might be to remove the
> restriction on the order in which the options can be specified.
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.
regards, tom lane
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..ae78c7b 100644
*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
*** init(bool is_no_vacuum)
*** 2844,2858
}
/*
! * Parse the raw sql and replace :param to $n.
*/
static bool
! parseQuery(Command *cmd, const char *raw_sql)
{
char *sql,
*p;
! sql = pg_strdup(raw_sql);
cmd->argc = 1;
p = sql;
--- 2844,2861
}
/*
! * Replace :param with $n throughout the command's SQL text, which
! * is a modifiable string in cmd->argv[0].
*/
static bool
! parseQuery(Command *cmd)
{
char *sql,
*p;
! /* We don't want to scribble on cmd->argv[0] until done */
! sql = pg_strdup(cmd->argv[0]);
!
cmd->argc = 1;
p = sql;
*** parseQuery(Command *cmd, const char *raw
*** 2874,2880
if (cmd->argc >= MAX_ARGS)
{
! fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n", MAX_ARGS - 1, raw_sql);
pg_free(name);
return false;
}
--- 2877,2884
if (cmd->argc >= MAX_ARGS)
{
! fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n",
! MAX_ARGS - 1, cmd->argv[0]);
pg_free(name);
return false;
}
*** parseQuery(Command *cmd, const char *raw
*** 2886,2891
--- 2890,2896
cmd->argc++;
}
+ pg_free(cmd->argv[0]);
cmd->argv[0] = sql;
return true;
}
*** process_sql_command(PQExpBuffer buf, con
*** 2983,2992
my_command = (Command *) pg_malloc0(sizeof(Command));
my_command->command_num = num_commands++;
my_command->type = SQL_COMMAND;
- my_command->argc = 0;
initSimpleStats(&my_command->stats);
/*
* If SQL command is multi-line, we only want to save the first line as
* the "line" label.
*/
--- 2988,3003
my_command = (Command *) pg_malloc0(sizeof(Command));
my_command->command_num = num_commands++;
my_command->type = SQL_COMMAND;
initSimpleStats(&my_command->stats);
/*
+ * Install query text as the sole argv string. If we are using a
+ * non-simple query mode, we'll extract parameters from it later.
+ */
+ my_command->argv[0] = pg_strdup(p);
+ my_command->argc = 1;
+
+ /*
* If SQL command is multi-line, we only want to save the first line as
* the "line" label.
*/
*** process_sql_command(PQExpBuffer buf, con
*** 3000,3020
else
my_command->line = pg_strdup(p);
- switch (querymode)
- {
- case QUERY_SIMPLE:
- my_command->argv[0] = pg_strdup(p);
- my_command->argc++;
- break;
- case QUERY_EXTENDED:
- case QUERY_PREPARED:
- if (!parseQuery(my_command, p))
- exit(1);
- break;
- default:
- exit(1);
- }
-
return my_command;
}
--- 3011,3016
*** main(int argc, char **argv)
*** 3896,3906
break;
case 'M':
benchmarking_option_set = true;
- if (num_scripts > 0)
- {
- fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
- exit(1);
- }
for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)
if (strcmp(optarg, QUERYMODE[querymode]) == 0)
break;
--- 3892,3897
*** main(int argc, char **argv)
*** 4006,4011
--- 3997,4020
internal_script_used = true;
}
+ /* if not simple query mode, parse the script(s) to find parameters */
+ if (querymode != QUERY_SIMPLE)
+ {
+ for (i = 0; i < num_scripts; i++)
+ {
+ Command **commands = sql_script[i].commands;
+ int j;
+
+ for (j = 0; commands[j] != NULL; j++)
+ {
+ if (commands[j]->type != SQL_COMMAND)
+ continue;
+ if (!parseQuery(commands[j]))
+ exit(1);
+ }
+ }
+ }
+
/* compute total_weight */
for (i = 0; i < num_scripts; i++)
/* cannot overflow: weight is 32b, total_weight 64b */
--
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] Function Volatility and Views Unexpected Behavior
On Wed, Jul 12, 2017 at 3:23 PM, Tom Lane wrote: > David Kohn writes: >> I encountered some unexpected behavior when debugging a query that was >> taking longer than expected, basically, a volatile function that makes a >> column in a view is called even when that column is not selected in the >> query, making it so that the function is called for every row in the view, >> I'm not sure that that would necessarily be the expected behavior, as it >> was my understanding that columns that are not selected are not evaluated, >> for instance if there was a join in a view that produced some columns and >> said columns were not selected, I would expect it to be optimized away. > > No, this is the expected behavior; we don't like optimization to change > the number of calls of a volatile function from what would occur in naive > evaluation of the query. If that prospect doesn't bother you, it's > likely because your function isn't really volatile ... I don't think I agree with that. If something is VOLATILE, that means you want it to be recalculated each time, but it doesn't necessarily mean that you want it calculated if it in no way changes the result set. I guess maybe there's a difference between a VOLATILE function like random(), which is expected to produce a different answer each time but probably has no side effects that you care about (unless you care about the fact that the state of the PRNG has changed) and pg_sleep(), whose return value is always the same but whose side effects are of critical importance. Maybe we need separate terms for volatile-because-the-answer-is-unstable and volatile-because-it-has-side-effects. -- 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] Gettting warning message during PostgreSQL-9.5 installation on Windows
Ashutosh Sharma writes: > I am getting this warning message when trying to install > PostgreSQL-v9.5 on Windows with Perl-5.22 and above, > Unescaped left brace in regex is deprecated, passed through in regex; > Please note that from perl-5.26 onwards, this is considered as a > syntax error instead of warning. Mmm, yeah, we'd better fix it then, because people will surely try to use older branches with current Perl. Pushed. 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
On Wed, Aug 2, 2017 at 1:58 PM, 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. Yeah, that's definitely confusing. I realize that my verbless version is a little odd, but there are numerous precedents for it: --inserts, --column-inserts, --if-exists, --strict-names, ... -- 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
On Wed, Aug 2, 2017 at 5:50 PM, Tom Lane wrote:
> Robert Haas writes:
>> On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane wrote:
>>> Or in other words, this looks to me quite a bit like the hackery
>>> that resulted in pgbench's -S and -N options, before we figured out
>>> that making it scriptable was a better answer.
>
>> But it's not very clear to me how we could make this case scriptable,
>
> Well, I'm imagining that "-i" would essentially become a short form
> of "-b initialize", as already happened for -S and -N, where the script
> looks something like
Yes, I would imagine a facility where one could do pgbench $script and
issue a complete test set. Here is for example a funky idea: let's
separate each script with a set of meta-commands, \init being what is
used just for initialization, and then use \script to define a set of
commands with a custom weight. Say:
\init
CREATE TABLE foo(a int);
\script select_query [weight N]
SELECT count(*) FROM foo;
\script insert_query [weight N]
INSERT INTO foo VALUES ('1');
That may be over-engineering things, but personally I don't like much
having just a switch to remove indexes. Next time we will come with
another option that only selects a portion of the indexes created.
--
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] On Complex Source Code Reading Strategy
On Wed, Aug 2, 2017 at 7:24 AM, Zeray Kalayu wrote: > Lastly, I strongly believe that Code is the ultimate truth and being > able to understand complex and high quality code effectively and > strategically is of paramount importance. Documentation to understand how a system works from the user prospective, and comments in the code itself are also important properties of a code that can be considered as a good base. Postgres has both. -- 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] reload-through-the-top-parent switch the partition table
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 don't know that we need to use "root" in the argument name to communicate "the top-most if partitioned tables are nested". We have the docs to describe exactly what it does. "Partitioned Table" is what we are calling the main routing table in the docs. "Use" seems adequate. FWIW my first thought was "--insert-via-partition-root" and I didn't mind that "COPY" commands would be affected implicitly. David J.
Re: [HACKERS] Macros bundling RELKIND_* conditions
Alvaro Herrera writes: > Peter Eisentraut wrote: >> The actual error, from the perspective of the user, is something like >> ERROR: "someview" is a view >> DETAIL: Views cannot have constraints. > OK. "%s is a %s" is a reasonable set of errors -- we just need one for > each relkind. So the first one is easy. > But the second one is not easy, because we'd need one message per > relkind per operation kind. We cannot possibly write/translate that > many messages. If we make the relkind generic in the errdetail message, > maybe it can work; something like "Relations of that type cannot have > constraints" would work, for example. Or "Relations of type "view" > cannot have constraints", although this reads very strangely. Maybe > someone has a better idea? 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. A more verbose approach is ERROR: "someview" cannot have constraints DETAIL: "someview" is a view, which is not a supported kind of relation for this purpose. 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] pg_stop_backup(wait_for_archive := true) on standby server
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Mon, Jul 31, 2017 at 9:13 PM, Stephen Frost wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > >> On Thu, Jul 27, 2017 at 10:27 AM, Stephen Frost wrote: > >> > * Noah Misch (n...@leadboat.com) wrote: > >> >> This PostgreSQL 10 open item is past due for your status update. > >> >> Kindly send > >> >> a status update within 24 hours, and include a date for your subsequent > >> >> status > >> >> update. Refer to the policy on open item ownership: > >> > > >> > Based on the ongoing discussion, this is really looking like it's > >> > actually a fix that needs to be back-patched to 9.6 rather than a PG10 > >> > open item. I don't have any issue with keeping it as an open item > >> > though, just mentioning it. I'll provide another status update on or > >> > before Monday, July 31st. > >> > > >> > I'll get to work on the back-patch and try to draft up something to go > >> > into the release notes for 9.6.4. > >> > >> Whether this is going to be back-patched or not, you should do > >> something about it quickly, because we're wrapping a new beta and a > >> full set of back-branch releases next week. I'm personally hoping > >> that what follows beta3 will be rc1, but if we have too much churn > >> after beta3 we'll end up with a beta4, which could end up slipping the > >> whole release cycle. > > > > Yes, I've been working on this and the other issues with pg_dump today. > > 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. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] reload-through-the-top-parent switch the partition table
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. 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] pg_stop_backup(wait_for_archive := true) on standby server
On Mon, Jul 31, 2017 at 9:13 PM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Thu, Jul 27, 2017 at 10:27 AM, Stephen Frost wrote: >> > * Noah Misch (n...@leadboat.com) wrote: >> >> This PostgreSQL 10 open item is past due for your status update. Kindly >> >> send >> >> a status update within 24 hours, and include a date for your subsequent >> >> status >> >> update. Refer to the policy on open item ownership: >> > >> > Based on the ongoing discussion, this is really looking like it's >> > actually a fix that needs to be back-patched to 9.6 rather than a PG10 >> > open item. I don't have any issue with keeping it as an open item >> > though, just mentioning it. I'll provide another status update on or >> > before Monday, July 31st. >> > >> > I'll get to work on the back-patch and try to draft up something to go >> > into the release notes for 9.6.4. >> >> Whether this is going to be back-patched or not, you should do >> something about it quickly, because we're wrapping a new beta and a >> full set of back-branch releases next week. I'm personally hoping >> that what follows beta3 will be rc1, but if we have too much churn >> after beta3 we'll end up with a beta4, which could end up slipping the >> whole release cycle. > > Yes, I've been working on this and the other issues with pg_dump today. Do you need a back-patchable version for 9.6? I could get one out of my pocket if necessary. -- 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] Macros bundling RELKIND_* conditions
Peter Eisentraut wrote: > I don't find this style of error message optimal anyway. If I do, for > example > > ALTER TABLE someview ADD CONSTRAINT ... > ERROR: "someview" is not a table, foreign table, whatever > > then this information is not helpful. It's not like I'm going to turn > my view into a foreign table in order to be able to proceed with that > command. Hmm, this is a good point ... not against my proposal, but rather against the current coding. I agree it could be more user-friendly. > The actual error, from the perspective of the user, is something like > > ERROR: "someview" is a view > DETAIL: Views cannot have constraints. OK. "%s is a %s" is a reasonable set of errors -- we just need one for each relkind. So the first one is easy. But the second one is not easy, because we'd need one message per relkind per operation kind. We cannot possibly write/translate that many messages. If we make the relkind generic in the errdetail message, maybe it can work; something like "Relations of that type cannot have constraints" would work, for example. Or "Relations of type "view" cannot have constraints", although this reads very strangely. Maybe someone has a better idea? -- Á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] reload-through-the-top-parent switch the partition table
On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane wrote: > Robert Haas writes: >> The patch itself looks just fine on a quick glance, modulo the lack of >> documentation, but I think we need to bikeshed the name of the flag. >> --reload-through-root is clear as daylight to me, but I'm not sure >> users will agree. The lack of the word "partition" is perhaps a >> significant flaw, and pg_dump doesn't really reload anything; it just >> dumps. > >> The best thing I can come up with after brief thought is >> --partition-data-via-root, but maybe somebody else has a better idea? > > --restore-via-partition-root ? I worry someone will think that pg_dump is now restoring stuff, but it isn't. -- 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
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. However, if there are multiple places that should be kept in sync regarding which relkinds to check, then I don't understand Robert's objection that only one place requires the check. Surely we're having this discussion precisely because more than one place needs the check, and finding those places is not obvious? -- Á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] Macros bundling RELKIND_* conditions
On 8/2/17 13:28, 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,
>
> {
> {
> relkinds_r_i_T,
> { 'r', 'i', 'T' },
> gettext_noop("relation %s is not a table, index or toast table")
> },
> ...
> }
I don't find this style of error message optimal anyway. If I do, for
example
ALTER TABLE someview ADD CONSTRAINT ...
ERROR: "someview" is not a table, foreign table, whatever
then this information is not helpful. It's not like I'm going to turn
my view into a foreign table in order to be able to proceed with that
command. The actual error, from the perspective of the user, is
something like
ERROR: "someview" is a view
DETAIL: Views cannot have constraints.
(Maybe they can. This is an example.)
--
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] why not parallel seq scan for slow functions
On Wed, Jul 12, 2017 at 7:08 PM, Amit Kapila wrote: > On Wed, Jul 12, 2017 at 11:20 PM, Jeff Janes wrote: > > On Tue, Jul 11, 2017 at 10:25 PM, Amit Kapila > > wrote: > >> > >> On Wed, Jul 12, 2017 at 1:50 AM, Jeff Janes > wrote: > >> > On Mon, Jul 10, 2017 at 9:51 PM, Dilip Kumar > >> > wrote: > >> >> > >> >> So because of this high projection cost the seqpath and parallel path > >> >> both have fuzzily same cost but seqpath is winning because it's > >> >> parallel safe. > >> > > >> > > >> > I think you are correct. However, unless parallel_tuple_cost is set > >> > very > >> > low, apply_projection_to_path never gets called with the Gather path > as > >> > an > >> > argument. It gets ruled out at some earlier stage, presumably because > >> > it > >> > assumes the projection step cannot make it win if it is already behind > >> > by > >> > enough. > >> > > >> > >> I think that is genuine because tuple communication cost is very high. > > > > > > Sorry, I don't know which you think is genuine, the early pruning or my > > complaint about the early pruning. > > > > Early pruning. See, currently, we don't have a way to maintain both > parallel and non-parallel paths till later stage and then decide which > one is better. If we want to maintain both parallel and non-parallel > paths, it can increase planning cost substantially in the case of > joins. Now, surely it can have benefit in many cases, so it is a > worthwhile direction to pursue. > If I understand it correctly, we have a way, it just can lead to exponential explosion problem, so we are afraid to use it, correct? If I just lobotomize the path domination code (make pathnode.c line 466 always test false) if (JJ_all_paths==0 && costcmp != COSTS_DIFFERENT) Then it keeps the parallel plan and later chooses to use it (after applying your other patch in this thread) as the overall best plan. It even doesn't slow down "make installcheck-parallel" by very much, which I guess just means the regression tests don't have a lot of complex joins. But what is an acceptable solution? Is there a heuristic for when retaining a parallel path could be helpful, the same way there is for fast-start paths? It seems like the best thing would be to include the evaluation costs in the first place at this step. Why is the path-cost domination code run before the cost of the function evaluation is included? Is that because the information needed to compute it is not available at that point, or because it would be too slow to include it at that point? Or just because no one thought it important to do? Cheers, Jeff
Re: [HACKERS] Walsender timeouts and large transactions
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
There is no check for (last_reply_timestamp <= 0 || wal_sender_timeout <= 0) as
in other places
(in WalSndKeepaliveIfNecessary for example).
I don't think moving update of 'now' down to end of loop body is correct:
there are calls to ProcessConfigFile with SyncRepInitConfig,
ProcessRepliesIfAny that can
last non-negligible time. It could lead to over sleeping due to larger computed
sleeptime.
Though I could be mistaken.
I'm not sure about moving `if (!pg_is_send_pending())` in a body loop after
WalSndKeepaliveIfNecessary.
Is it necessary? But it looks harmless at least.
Could patch be reduced to check after first `if (!pg_is_sendpending())` ? like:
if (!pq_is_send_pending())
- return;
+ {
+ if (last_reply_timestamp <= 0 || wal_sender_timeout <= 0)
+ {
+ CHECK_FOR_INTERRUPTS();
+ return;
+ }
+ if (now <= TimestampTzPlusMilliseconds(last_reply_timestamp,
wal_sender_timeout / 2))
+ return;
+ }
If not, what problem prevents?
The new status of this patch is: Waiting on Author
--
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
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,
{
{
relkinds_r_i_T,
{ 'r', 'i', 'T' },
gettext_noop("relation %s is not a table, index or toast table")
},
...
}
and then in the .c checks you do something like
relkinds = relkind_r_i_T;
if (rel_doesnt_match(rel, relkinds))
ereport(ERROR, (errcode(...),
errmsg(relkinds_get_message(relkinds)));
then, in order to update the set of relkinds that some particular
operation works with, you just need to change the "relkinds" variable,
and the message is automatically up to date (you may need to add a new
entry, if there isn't one for the set you want, but the number of
permutations needed shouldn't grow too large). This doesn't create a
problem for translators because you're not constructing an error
message, and it doesn't pollute pg_class.h with things that don't really
belong there.
One possible objection is that rel_doesnt_match() in the above
formulation is slow, because it has to scan the entire list. Maybe this
is not a problem because the list isn't large anyway, or maybe there's
some better formulation -- for example, we could assign a distinct bit
to each relkind, and create bitmasks for each set; then figuring out
whether there's a match or not is just a matter of bit-anding.
--
Á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] reload-through-the-top-parent switch the partition table
Robert Haas writes: > The patch itself looks just fine on a quick glance, modulo the lack of > documentation, but I think we need to bikeshed the name of the flag. > --reload-through-root is clear as daylight to me, but I'm not sure > users will agree. The lack of the word "partition" is perhaps a > significant flaw, and pg_dump doesn't really reload anything; it just > dumps. > The best thing I can come up with after brief thought is > --partition-data-via-root, but maybe somebody else has a better idea? --restore-via-partition-root ? 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] pgbench: Skipping the creating primary keys after initialization
On Wed, Aug 2, 2017 at 12:34 PM, Tom Lane wrote: > Of course. It's also a heck of a lot more flexible. Adding on another > ad-hoc option that does the minimum possible amount of work needed to > address one specific problem is always going to be less work; but after > we repeat that process five or ten times, we're going to have a mess. Well, I still like Masahiko-san's proposal, but I'm not prepared to keep arguing about it right now. Maybe some other people will weigh in with an opinion. -- 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
Robert Haas writes: > I've thought about this kind of thing, too. But the thing is that > most of these macros you're proposing to introduce only get used in > one place. I think the value would be in having a centralized checklist of things-to-fix-when-adding-a-new-relkind. There's more than one way to reach that goal, though. I wonder whether the task should be defined more like "grep for 'RELKIND_' and fix every place you find that". If there are places to touch that fail to mention that string, fix them, using comments if nothing else. (But see fe797b4a6 and followon commits for other solutions.) > I think this might cause some problems for translators. Yeah, the error messages that list a bunch of different relkinds in text form are going to be a hassle no matter what. Most of the ways you might think of to change that will violate our translatability rules. 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] Re: [BUGS] BUG #14758: Segfault with logical replication on a function index
On 8/1/17 16:29, Peter Eisentraut wrote: > On 8/1/17 00:21, Noah Misch wrote: >> On Mon, Jul 31, 2017 at 09:40:34AM +0900, Masahiko Sawada wrote: >>> On Sat, Jul 29, 2017 at 4:35 AM, Scott Milliken wrote: Thank you Masahiko! I've tested and confirmed that this patch fixes the problem. >>> >>> Thank you for the testing. This issue should be added to the open item >>> since this cause of the server crash. I'll add it. >> >> [Action required within three days. This is a generic notification.] > > I'm looking into this now and will report back on Thursday. This item has been closed. -- 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] pgbench: Skipping the creating primary keys after initialization
Robert Haas writes: > On Wed, Aug 2, 2017 at 11:50 AM, Tom Lane wrote: >> Well, I'm imagining that "-i" would essentially become a short form >> of "-b initialize", as already happened for -S and -N, where the script >> looks something like ... > I imagine that would be useful for some use cases, but it's a heck of > a lot more work than just writing --no-indexes-please. Of course. It's also a heck of a lot more flexible. Adding on another ad-hoc option that does the minimum possible amount of work needed to address one specific problem is always going to be less work; but after we repeat that process five or ten times, we're going to have a mess. 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] pgbench: Skipping the creating primary keys after initialization
On Wed, Aug 2, 2017 at 11:50 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane wrote: >>> Or in other words, this looks to me quite a bit like the hackery >>> that resulted in pgbench's -S and -N options, before we figured out >>> that making it scriptable was a better answer. > >> But it's not very clear to me how we could make this case scriptable, > > Well, I'm imagining that "-i" would essentially become a short form > of "-b initialize", as already happened for -S and -N, where the script > looks something like > > drop table if exists pgbench_branches; > create table pgbench_branches ( > bid int not null,bbalance int,filler char(88) > ); > \load_data pgbench_branches [ other parameters to-be-determined ] > alter table pgbench_branches add primary key (bid); > ... repeat for other tables ... > > and we'd document that the same way we do for the existing built-in > scripts. Then, if there's something you don't like about it, you > just paste the script into a file and edit to taste. I imagine that would be useful for some use cases, but it's a heck of a lot more work than just writing --no-indexes-please. -- 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
Robert Haas writes: > On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane wrote: >> Or in other words, this looks to me quite a bit like the hackery >> that resulted in pgbench's -S and -N options, before we figured out >> that making it scriptable was a better answer. > But it's not very clear to me how we could make this case scriptable, Well, I'm imagining that "-i" would essentially become a short form of "-b initialize", as already happened for -S and -N, where the script looks something like drop table if exists pgbench_branches; create table pgbench_branches ( bid int not null,bbalance int,filler char(88) ); \load_data pgbench_branches [ other parameters to-be-determined ] alter table pgbench_branches add primary key (bid); ... repeat for other tables ... and we'd document that the same way we do for the existing built-in scripts. Then, if there's something you don't like about it, you just paste the script into a file and edit to taste. I'm sure there's complexities that would only become apparent when someone tries to write the patch, but that seems to me like a better foundation for this class of desires than extending the option set with various one-off options having no discernible architecture. > If you just want to create > different/extra indexes, you can do that yourself. Sure, but there's no end to the number of small variations on this theme that somebody might want. For example, we realized years ago that the "filler" fields as-implemented don't really meet the intent of the TPC-B spec (cf comment in the init() function). If someone comes along with a patch adding a "--really-tpc-b" option to change the table declarations and/or data loading code to fix that, will we take that patch? What about one that wants all the id fields (not just accounts.aid) to be bigint, or one that wants the balance fields to be numeric? You can say "let 'em set up the tables manually if they want that", but I don't see why a nonstandard set of indexes is much different. 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, 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. I suppose we could still try to jam that change in before beta3 (bumping HASH_VERSION again) but that might be asking for trouble. -- 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] Confusing error message in pgbench
Hello Tatsuo-san,
I found an error message in pgbench is quite confusing.
pgbench -S -M extended -c 1 -T 30 test
query mode (-M) should be specified before any transaction scripts (-f or -b)
Since there's no -f or -b option is specified, users will be
confused.
Indeed.
Actually the error occurs because pgbench implicitly introduces a built
in script for -S. To eliminate the confusion, I think the error message
should be fixed like this:
The idea is that -S/-N documentations say that it is just a shortcut for
-b, but the explanation (eg --help) is too far away.
query mode (-M) should be specified before transaction type (-S or -N)
or any transaction scripts (-f or -b)
I would suggest to make it even shorter, see attached:
query mode (-M) should be specified before any transaction scripts (-f,
-b, -S or -N).
I'm wondering whether it could/should be "any transaction script". My
English level does not allow to decide.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..9e41c07 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3898,7 +3898,7 @@ main(int argc, char **argv)
benchmarking_option_set = true;
if (num_scripts > 0)
{
- fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
+ fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f, -b, -S or -N)\n");
exit(1);
}
for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)
--
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 Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat
wrote:
>> I noticed, that
>> after we introduced RELKIND_PARTITIONED_TABLE, we required to change a
>> number of conditions to include this relkind. We missed some places in
>> initial commits and fixed those later. I am wondering whether we
>> should creates macros clubbing relevant relkinds together based on the
>> purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind
>> is added, one can examine these macros to check whether the new
>> relkind fits in the given macro. If all those macros are placed
>> together, there is a high chance that we will not miss any place in
>> the initial commit itself.
I've thought about this kind of thing, too. But the thing is that
most of these macros you're proposing to introduce only get used in
one place.
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.
> With this approach the macro which tests relkinds and the macro which
> reports error are places together in pg_class.h. If somebody adds a
> new relkind, s/he will notice the comment there and update the macros
> below also keeping the error message in sync with the test. Please
> note that partitioned tables are not explicitly mentioned in the error
> messages when the corresponding test has RELKIND_PARTITIONED_TABLE. I
> think we don't need to differentiate between a regular table and
> partitioned table in those error messages; a "table" implies both a
> regular table and a partitioned table.
I'm honestly not sure this buys us anything, unless you can use those
macros in a lot more places.
> With this approach, if a developer may still fail to update the error
> message when the test is updated. We can further tighten this by
> following approach.
> 1. For every test declare an array of relkinds that the test accepts e.g.
> int relkinds_with_vm[] = {RELKIND_RELATION, RELKIND_MATVIEW,
> RELKIND_TOASTVALUE};
> 2. Write a function is_relkind_in_array(int *relkinds_array, int
> num_relkinds, int relkind) to check whether the given relkind is in
> the array.
> 3. Each test macro now calls this function passing appropriate array
> e.g. #define RELKIND_WITH_VISIBILITY_MAP(relkind) \
> is_relkind_in_array(relkinds_with_vm,
> sizeof(relkinds_with_vm)/sizeof(relkinds_with_vm[0], (relkind))
> 4. Declare an array of relkinds and their readable strings e.g
> {{RELKIND_RELATION, "table"}, {RELKIND_MATVIEW, "materialized view"}}
> 5. Write a function to collect the readable strings for all relkinds a
> given array of relkinds say char *relkind_names(int *relkinds, int
> num_relkinds)
> 6. Declare error message macros to call this function by passing
> appropriate array.
I think this might cause some problems for translators.
--
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] [TRAP: FailedAssertion] causing server to crash
On Fri, Jul 21, 2017 at 1:31 AM, Thomas Munro wrote: > Thanks Neha. It's be best to post the back trace and if possible > print oldestXact and ShmemVariableCache->oldestXid from the stack > frame for TruncateCLOG. > > The failing assertion in TruncateCLOG() has a comment that says > "vac_truncate_clog already advanced oldestXid", but vac_truncate_clog > calls SetTransactionIdLimit() to write ShmemVariableCache->oldestXid > *after* it calls TruncateCLOG(). What am I missing here? This problem was introduced by commit ea42cc18c35381f639d45628d792e790ff39e271, so this should be added to the PostgreSQL 10 open items list. That commit intended to introduce a distinction between (1) the oldest XID that can be safely examined and (2) the oldest XID that can't yet be safely reused. These are the same except when we're in the middle of truncating CLOG: (1) advances before the truncation, and (2) advances afterwards. That's why AdvanceOldestClogXid() happens before truncation proper and SetTransactionIdLimit() happens afterwards, and changing the order would, I think, be quite wrong. AFAICS, that assertion is simply a holdover from an earlier version of the patch that escaped review. There's just no reason to suppose that it's true. > What actually prevents ShmemVariableCache->oldestXid from going > backwards anyway? Suppose there are two or more autovacuum processes > that reach vac_truncate_clog() concurrently. They do a scan of > pg_database whose tuples they access without locking through a > pointer-to-volatile because they expect concurrent in-place writers, > come up with a value for frozenXID, and then arrive at > SetTransactionIdLimit() in whatever order and clobber > ShmemVariableCache->oldestXid. What am I missing here? Hmm, there could be a bug there, but I don't think it's *this* bug. -- 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] typo for using "OBJECT_TYPE" for "security label on domain" in "gram.y"
On 8/2/17 08:21, Robert Haas wrote: > On Wed, Aug 2, 2017 at 6:04 AM, 高增琦 wrote: >> Commit: 3f88672a4e4d8e648d24ccc65937da61c7660854 add "security label on >> domain" >> in "gram.y", and set "objtype" to "OBJECT_TYPE". >> >> Is this a typo? > > Looks like it. Fix committed to master. I don't intend to backpatch it. -- 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] pgbench: Skipping the creating primary keys after initialization
On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane wrote: > Sure, but "no indexes at all" is hardly ever the real goal, is it? Right. > So the switch as proposed is only solving part of your problem. > I'd rather see a solution that addresses a larger range of desires. That's reasonable. > Or in other words, this looks to me quite a bit like the hackery > that resulted in pgbench's -S and -N options, before we figured out > that making it scriptable was a better answer. But it's not very clear to me how we could make this case scriptable, and it would probably not be much different from just using the proposed option and then running the script afterwards yourself via psql. The thing about -N and -S is that those scripts are being run repeatedly, so pgbench has to be involved. If you just want to create different/extra indexes, you can do that yourself. -- 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] Unused variable scanned_tuples in LVRelStats
On Tue, Jul 4, 2017 at 10:13 PM, Masahiko Sawada wrote: > scanned_tuples variable in LVRelStats is introduced by commit b4b6923e > but it seems to me that it's actually not used. We store num_tuples > into vacrelstats->scanned_tuples after scanned all blocks, and the > comment mentioned that saving it in order to use later but we actually > use num_tuples instead of vacrelstats->scanned_tuples from there. I > think the since the name of scanned_tuples implies more clearer > purpose than num_tuples it's better to use it instead of num_tuples, > or we can remove scanned_tuples from LVRelStats. I think we should only store stuff in LVRelStats if it needs to be passed to some other function. Data that's only used in lazy_scan_heap() can just be kept in local variables. We could rename the local variable, though, since I agree with you that scanned_tuples is clearer. -- 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] Domains and arrays and composites, oh my
On Thu, Jul 13, 2017 at 3:42 PM, Tom Lane wrote: > Yeah, it does, although I'm not sure how intuitive it is that the > parentheses are significant ... > > regression=# select fdc.* from fdc(); > fdc > --- > (1,2) > (1 row) > > regression=# select (fdc).* from fdc(); > r | i > ---+--- > 1 | 2 > (1 row) Not intuitive at all. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Robert Haas writes: > I've actually wanted this exact thing multiple times: most recently, > to make a non-unique btree index instead of a unique one, and to make > a hash index instead of a btree one. I don't object to a modest > effort at coming up with a more general mechanism here, but I also > think the switch as proposed is something that would have met my real > needs on multiple occasions. I've probably had 10 different occasions > when I wanted all of the standard pgbench initialization *except for* > something different about the indexes. Sure, but "no indexes at all" is hardly ever the real goal, is it? So the switch as proposed is only solving part of your problem. I'd rather see a solution that addresses a larger range of desires. Or in other words, this looks to me quite a bit like the hackery that resulted in pgbench's -S and -N options, before we figured out that making it scriptable was a better answer. 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
On Wed, Aug 2, 2017 at 1:01 AM, Rushabh Lathia wrote: > Looking at the dbObjectTypePriority comments that seems like data > restoration > will *absolutely always* follow all CREATE TABLE commands. Hmm. I wasn't very convinced by those comments, but Tom's commit a1ef01fe163b304760088e3e30eb22036910a495 convinces me that it has to work that way. So I think we are OK on that score. The patch itself looks just fine on a quick glance, modulo the lack of documentation, but I think we need to bikeshed the name of the flag. --reload-through-root is clear as daylight to me, but I'm not sure users will agree. The lack of the word "partition" is perhaps a significant flaw, and pg_dump doesn't really reload anything; it just dumps. The best thing I can come up with after brief thought is --partition-data-via-root, but maybe somebody else has a better idea? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
On Wed, Aug 2, 2017 at 9:41 AM, Tom Lane wrote: > Robert Haas writes: >> On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada >> wrote: >>> I'd like to propose a new option -I for pgbench command which skips >>> the creating primary keys after initialized tables. > >> I support adding an option for this, but I propose that we just make >> it a long-form option, similar to --log-prefix or --index-tablespace. > > I think we could probably do without this ... if you want a non-default > test setup, why do you need to use "pgbench -i" to create it? > > It's not that there's anything greatly wrong with this particular idea, > it's just that pgbench has too many switches already, and omitting random > subsets of the initialization actions doesn't seem like it contributes > fundamental new benchmarking capability. > > I could get behind a proposal that generalized pgbench's "-i" behavior > in some meaningful way. I wonder whether it would be possible to convert > that behavior into a script. Some of what it does is just SQL commands > with injected parameters, which pgbench does already. There's also > data-loading actions, which could be converted to backslash commands > perhaps. Then desires like this could be addressed by invoking a > customized script instead of complicating pgbench's option set. I've actually wanted this exact thing multiple times: most recently, to make a non-unique btree index instead of a unique one, and to make a hash index instead of a btree one. I don't object to a modest effort at coming up with a more general mechanism here, but I also think the switch as proposed is something that would have met my real needs on multiple occasions. I've probably had 10 different occasions when I wanted all of the standard pgbench initialization *except for* something different about the indexes. -- 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
> I think we could probably do without this ... if you want a non-default > test setup, why do you need to use "pgbench -i" to create it? > > It's not that there's anything greatly wrong with this particular idea, > it's just that pgbench has too many switches already, and omitting random > subsets of the initialization actions doesn't seem like it contributes > fundamental new benchmarking capability. > > I could get behind a proposal that generalized pgbench's "-i" behavior > in some meaningful way. I wonder whether it would be possible to convert > that behavior into a script. Some of what it does is just SQL commands > with injected parameters, which pgbench does already. There's also > data-loading actions, which could be converted to backslash commands > perhaps. Then desires like this could be addressed by invoking a > customized script instead of complicating pgbench's option set. +1. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Robert Haas writes: > On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada wrote: >> I'd like to propose a new option -I for pgbench command which skips >> the creating primary keys after initialized tables. > I support adding an option for this, but I propose that we just make > it a long-form option, similar to --log-prefix or --index-tablespace. I think we could probably do without this ... if you want a non-default test setup, why do you need to use "pgbench -i" to create it? It's not that there's anything greatly wrong with this particular idea, it's just that pgbench has too many switches already, and omitting random subsets of the initialization actions doesn't seem like it contributes fundamental new benchmarking capability. I could get behind a proposal that generalized pgbench's "-i" behavior in some meaningful way. I wonder whether it would be possible to convert that behavior into a script. Some of what it does is just SQL commands with injected parameters, which pgbench does already. There's also data-loading actions, which could be converted to backslash commands perhaps. Then desires like this could be addressed by invoking a customized script instead of complicating pgbench's option set. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
On Wed, Aug 2, 2017 at 10:25 PM, Robert Haas wrote: > On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada wrote: >> I'd like to propose a new option -I for pgbench command which skips >> the creating primary keys after initialized tables. This option is >> useful for users who want to do bench marking with no index or indexes >> other than btree primary index. If we initialize pgbench tables at a >> large number scale factor the primary key index creation takes a long >> time even if we're going to use other types of indexes. With this >> option, the initialization time is reduced and you can create indexes >> as you want. >> >> Feedback is very welcome. I'll add this patch to the next CF. > > I support adding an option for this, but I propose that we just make > it a long-form option, similar to --log-prefix or --index-tablespace. > Yeah, that's better. I'll update the patch. 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] pgbench: Skipping the creating primary keys after initialization
On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada wrote: > I'd like to propose a new option -I for pgbench command which skips > the creating primary keys after initialized tables. This option is > useful for users who want to do bench marking with no index or indexes > other than btree primary index. If we initialize pgbench tables at a > large number scale factor the primary key index creation takes a long > time even if we're going to use other types of indexes. With this > option, the initialization time is reduced and you can create indexes > as you want. > > Feedback is very welcome. I'll add this patch to the next CF. I support adding an option for this, but I propose that we just make it a long-form option, similar to --log-prefix or --index-tablespace. -- 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] Confusing error message in pgbench
On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii wrote: > I found an error message in pgbench is quite confusing. > > pgbench -S -M extended -c 1 -T 30 test > query mode (-M) should be specified before any transaction scripts (-f or -b) > > Since there's no -f or -b option is specified, users will be > confused. Actually the error occurs because pgbench implicitly > introduces a built in script for -S. To eliminate the confusion, I > think the error message should be fixed like this: > > query mode (-M) should be specified before transaction type (-S or -N) or any > transaction scripts (-f or -b) > > Patch attached. Not really objecting, but an even better fix might be to remove the restriction on the order in which the options can be specified. -- 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] typo for using "OBJECT_TYPE" for "security label on domain" in "gram.y"
On Wed, Aug 2, 2017 at 6:04 AM, 高增琦 wrote: > Commit: 3f88672a4e4d8e648d24ccc65937da61c7660854 add "security label on > domain" > in "gram.y", and set "objtype" to "OBJECT_TYPE". > > Is this a typo? Looks like 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] Red-Black tree traversal tests
I forgot to attach the patch. Sorry.
Here it is.
--
--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 3ce9904..b7ed0af 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
test_extensions \
test_parser \
test_pg_dump \
+ test_rbtree \
test_rls_hooks \
test_shm_mq \
worker_spi
diff --git a/src/test/modules/test_rbtree/.gitignore b/src/test/modules/test_rbtree/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_rbtree/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_rbtree/Makefile b/src/test/modules/test_rbtree/Makefile
new file mode 100644
index 000..11a10cf
--- /dev/null
+++ b/src/test/modules/test_rbtree/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_rbtree/Makefile
+
+MODULE_big = test_rbtree
+OBJS = test.o $(WIN32RES)
+PGFILEDESC = "test_rbtree - rbtree triversal testing"
+
+EXTENSION = test_rbtree
+DATA = test_rbtree--1.0.sql
+
+REGRESS = test_rbtree
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_rbtree
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_rbtree/README b/src/test/modules/test_rbtree/README
new file mode 100644
index 000..8f4287e
--- /dev/null
+++ b/src/test/modules/test_rbtree/README
@@ -0,0 +1,20 @@
+test_rbtree is a module tests for checking the correctness of all kinds of
+traversal of red-black tree. Right now rbtree in postgres has 4 kinds of
+traversals: Left-Current-Right, Right-Current-Left, Current-Left-Right and
+Left-Right-Current.
+
+This extention has 4 functions. Each function checks one traversal.
+The checking the correctness of first two types are based on the fact that
+red-black tree is a binary search tree, so the elements should be iterated in
+increasing(for Left-Current-Right) or decreasing(for Right-Current-Left)
+order.
+In order to verify last two strategies, we will check the sequence if it is
+correct or not. For given pre- or post- order traversing of binary search tree
+it is always possible to say is it correct or not and to rebuild original tree.
+The idea is based on the fact that in such traversal sequence is always
+possible to determine current node, left subtree and right subtree.
+
+Also, this module is checking the correctness of the find, delete and leftmost
+operation.
+
+These tests are performed on red-black trees that store integers.
\ No newline at end of file
diff --git a/src/test/modules/test_rbtree/expected/test_rbtree.out b/src/test/modules/test_rbtree/expected/test_rbtree.out
new file mode 100644
index 000..cd4435b
--- /dev/null
+++ b/src/test/modules/test_rbtree/expected/test_rbtree.out
@@ -0,0 +1,43 @@
+CREATE EXTENSION test_rbtree;
+SELECT testleftright();
+ testleftright
+---
+
+(1 row)
+
+SELECT testrightleft();
+ testrightleft
+---
+
+(1 row)
+
+SELECT testdirect();
+ testdirect
+
+
+(1 row)
+
+SELECT testinverted();
+ testinverted
+--
+
+(1 row)
+
+SELECT testfind();
+ testfind
+--
+
+(1 row)
+
+SELECT testleftmost();
+ testleftmost
+--
+
+(1 row)
+
+SELECT testdelete();
+ testdelete
+
+
+(1 row)
+
diff --git a/src/test/modules/test_rbtree/int_rbtree.h b/src/test/modules/test_rbtree/int_rbtree.h
new file mode 100644
index 000..d153616
--- /dev/null
+++ b/src/test/modules/test_rbtree/int_rbtree.h
@@ -0,0 +1,49 @@
+/*--
+ *
+ * int_rbtree.h
+ * Definitions for integer red-black tree
+ *
+ * Copyright (c) 2013-2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_rbtree/int_rbtree.h
+ *
+ * -
+ */
+
+#ifndef INT_RBTREE_H
+#define INT_RBTREE_H
+
+#include "lib/rbtree.h"
+
+typedef struct IntRBTreeNode
+{
+ RBNode rbnode;
+ int key;
+
+} IntRBTreeNode;
+
+static int
+cmp(const RBNode *a, const RBNode *b, void *arg)
+{
+ const IntRBTreeNode *ea = (const IntRBTreeNode *) a;
+ const IntRBTreeNode *eb = (const IntRBTreeNode *) b;
+
+ return ea->key - eb->key;
+}
+
+static RBNode *
+alloc(void *arg)
+{
+ IntRBTreeNode *ea;
+ ea = malloc(sizeof(IntRBTreeNode));
+ return (RBNode *) ea;
+}
+
+static void
+fr(RBNode * node, void *arg)
+{
+ free(node);
+}
+
+#endif // INT_RBTREE_H
diff --git a/src/test/modules/test_rbtree/sql/test_rbtree.sql b/src/test/modules/test_rbtree/sql/test_rbtree.sql
new file mode 100644
index 000..3bedff2
--- /dev/null
+++ b/src/test/modules/test_rbtree/sql/test_rbtree.sql
@@ -0,0 +1,9 @@
+CREATE EXTENSION test
Re: [HACKERS] Red-Black tree traversal tests
Hello, Thank you for the reviewing. If it's not too much trouble perhaps you could write a few more test so we would have 100% test coverage for rbtree, could modify it safely and be sure that it actually works when someone will need the rest of its functionality? Done. Now all of the functions in rbtree.c are covered. Also I would recommend to add your patch to the nearest commitfest [1]. Otherwise there is a good chance that everyone will forget about it quite soon. [1]: https://commitfest.postgresql.org/14/ Done. Here is the link: https://commitfest.postgresql.org/14/1225/ Thank you for attention! -- -- Victor Drobny Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal for CSN based snapshots
What problem exactly you are seeing in the clog, is it the contention around CLOGControlLock or generally accessing CLOG is slower. If former, then we already have a patch [1] to address it. It's the contention around CLogControlLock. Thank you for the pointer, next time I'll try it with the group clog update patch. -- Alexander Kuzmenkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign table creation and NOT VALID check constraints
On Wed, Aug 2, 2017 at 3:46 AM, Ashutosh Bapat wrote: > If the user has specified "not valid" for a constraint on the foreign > table, there is high chance that s/he is aware of the fact that the > remote table that the foreign table points to has some rows which will > violet the constraint. So, +1. +1 from me, too. -- 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] A bug in mapping attributes in ATExecAttachPartition()
On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote wrote: > I too dislike the shape of attachRel. How about we rename attachRel to > attachrel? So, attachrel_children, attachrel_constr, etc. It's still > long though... :) OK, I can live with that, I guess. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] UPDATE of partition key
On 2 August 2017 at 14:38, Amit Langote wrote: > On 2017/07/29 2:45, Amit Khandekar wrote: >> On 28 July 2017 at 20:10, Robert Haas wrote: >>> On Wed, Jul 26, 2017 at 2:13 AM, Amit Langote wrote: I checked that we get the same result relation order with both the patches, but I would like to highlight a notable difference here between the approaches taken by our patches. In my patch, I have now taught RelationGetPartitionDispatchInfo() to lock *only* the partitioned tables in the tree, because we need to look at its partition descriptor to collect partition OIDs and bounds. We can defer locking (and opening the relation descriptor of) leaf partitions to a point where planner has determined that the partition will be accessed after all (not pruned), which will be done in a separate patch of course. >>> >>> That's very desirable, but I believe it introduces a deadlock risk >>> which Amit's patch avoids. A transaction using the code you've >>> written here is eventually going to lock all partitions, BUT it's >>> going to move the partitioned ones to the front of the locking order >>> vs. what find_all_inheritors would do. So, when multi-level >>> partitioning is in use, I think it could happen that some other >>> transaction is accessing the table using a different code path that >>> uses the find_all_inheritors order without modification. If those >>> locks conflict (e.g. query vs. DROP) then there's a deadlock risk. >> >> Yes, I agree. Even with single-level partitioning, the leaf partitions >> ordered by find_all_inheritors() is by oid values, so that's also >> going to be differently ordered. > > We do require to lock the parent first in any case. Doesn't that prevent > deadlocks by imparting an implicit order on locking by operations whose > locks conflict. Yes may be, but I am not too sure at this point. find_all_inheritors() locks only the children, and the parent lock is already locked separately. find_all_inheritors() does not necessitate to lock the children with the same lockmode as the parent. > Having said that, I think it would be desirable for all code paths to > manipulate partitions in the same order. For partitioned tables, I think > we can make it the partition bound order by replacing all calls to > find_all_inheritors and find_inheritance_children on partitioned table > parents with something else that reads partition OIDs from the relcache > (PartitionDesc) and traverses the partition tree breadth-first manner. > >>> Unfortunately I don't see any easy way around that problem, but maybe >>> somebody else has an idea. >> >> One approach I had considered was to have find_inheritance_children() >> itself lock the children in bound order, so that everyone will have >> bound-ordered oids, but that would be too expensive since it requires >> opening all partitioned tables to initialize partition descriptors. In >> find_inheritance_children(), we get all oids without opening any >> tables. But now that I think more of it, it's only the partitioned >> tables that we have to open, not the leaf partitions; and furthermore, >> I didn't see calls to find_inheritance_children() and >> find_all_inheritors() in performance-critical code, except in >> expand_inherited_rtentry(). All of them are in DDL commands; but yes, >> that can change in the future. > > This approach more or less amounts to calling the new > RelationGetPartitionDispatchInfo() (per my proposed patch, a version of > which I posted upthread.) Maybe we can add a wrapper on top, say, > get_all_partition_oids() which throws away other things that > RelationGetPartitionDispatchInfo() returned. In addition it locks all the > partitions that are returned, unlike only the partitioned ones, which is > what RelationGetPartitionDispatchInfo() has been taught to do. So there are three different task items here : 1. Arrange the oids in consistent order everywhere. 2. Prepare the Partition Dispatch Info data structure in the planner as against during execution. 3. For update tuple routing, assume that the result rels are ordered consistently to make the searching efficient. #3 depends on #1. So for that, I have come up with a minimum set of changes to have expand_inherited_rtentry() generate the rels in bound order. When we do #2 , it may be possible that we may need to re-do my changes in expand_inherited_rtentry(), but those are minimum. We may even end up having the walker function being used at multiple places, but right now it is not certain. So, I think we can continue the discussion about #1 and #2 in a separate thread. > >> Regarding dynamically locking specific partitions as and when needed, >> I think this method inherently has the issue of deadlock because the >> order would be random. So it feels like there is no way around other >> than to lock all partitions beforehand. > > I'm not sure why the order has to be random. If and when we decide to > open and lock a subset of part
Re: [HACKERS] POC: Cache data in GetSnapshotData()
On Wed, Aug 2, 2017 at 3:42 PM, Mithun Cy wrote: Sorry, there was an unnecessary header included in proc.c which should be removed adding the corrected patch. -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com Cache_data_in_GetSnapshotData_POC_04.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] POC: Cache data in GetSnapshotData()
I have made few more changes with the new patch. 1. Ran pgindent. 2. Instead of an atomic state variable to make only one process cache the snapshot in shared memory, I have used conditional try lwlock. With this, we have a small and reliable code. 3. Performance benchmarking Machine - cthulhu == [mithun.cy@cthulhu bin]$ lscpu Architecture: x86_64 CPU op-mode(s):32-bit, 64-bit Byte Order:Little Endian CPU(s):128 On-line CPU(s) list: 0-127 Thread(s) per core:2 Core(s) per socket:8 Socket(s): 8 NUMA node(s): 8 Vendor ID: GenuineIntel CPU family:6 Model: 47 Model name:Intel(R) Xeon(R) CPU E7- 8830 @ 2.13GHz Stepping: 2 CPU MHz: 1197.000 BogoMIPS: 4266.63 Virtualization:VT-x L1d cache: 32K L1i cache: 32K L2 cache: 256K L3 cache: 24576K NUMA node0 CPU(s): 0,65-71,96-103 NUMA node1 CPU(s): 72-79,104-111 NUMA node2 CPU(s): 80-87,112-119 NUMA node3 CPU(s): 88-95,120-127 NUMA node4 CPU(s): 1-8,33-40 NUMA node5 CPU(s): 9-16,41-48 NUMA node6 CPU(s): 17-24,49-56 NUMA node7 CPU(s): 25-32,57-64 Server configuration: ./postgres -c shared_buffers=8GB -N 300 -c min_wal_size=15GB -c max_wal_size=20GB -c checkpoint_timeout=900 -c maintenance_work_mem=1GB -c checkpoint_completion_target=0.9 -c wal_buffers=256MB & pgbench configuration: scale_factor = 300 ./pgbench -c $threads -j $threads -T $time_for_reading -M prepared -S postgres The machine has 64 cores with this patch I can see server starts improvement after 64 clients. I have tested up to 256 clients. Which shows performance improvement nearly max 39%. Alternatively, I thought instead of storing the snapshot in a shared memory each backend can hold on to its previously computed snapshot until next commit/rollback happens in the system. We can have a global counter value associated with the snapshot when ever it is computed. Upon any new end of the transaction, the global counter will be incremented. So when a process wants a new snapshot it can compare these 2 values to check if it can use previously computed snapshot. This makes code significantly simple. With the first approach, one process has to compute and store the snapshot for every end of the transaction and others can reuse the cached the snapshot. In the second approach, every process has to re-compute the snapshot. So I am keeping with the same approach. On Mon, Jul 10, 2017 at 10:13 AM, Mithun Cy wrote: > On Fri, Apr 8, 2016 at 12:13 PM, Robert Haas wrote: >> I think that we really shouldn't do anything about this patch until >> after the CLOG stuff is settled, which it isn't yet. So I'm going to >> mark this Returned with Feedback; let's reconsider it for 9.7. > > I am updating a rebased patch have tried to benchmark again could see > good improvement in the pgbench read-only case at very high clients on > our cthulhu (8 nodes, 128 hyper thread machines) and power2 (4 nodes, > 192 hyper threads) machine. There is some issue with base code > benchmarking which is somehow not consistent so once I could figure > out what is the issue with that I will update > > -- > Thanks and Regards > Mithun C Y > EnterpriseDB: http://www.enterprisedb.com -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com cache_the_snapshot_performance.ods Description: application/vnd.oasis.opendocument.spreadsheet Cache_data_in_GetSnapshotData_POC_03.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] Parallel Hash take II
On Tue, Aug 1, 2017 at 1:11 PM, Andres Freund wrote: > WRT the main patch: Thanks for the review. I will respond soon, but for now I just wanted to post a rebased version (no changes) because v16 no longer applies. -- Thomas Munro http://www.enterprisedb.com parallel-hash-v17.patchset.tgz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] INSERT ON CONFLICT and partitioned tables
I applied the patch on latest master sources and the patch applies cleanly. The documentation is built without errors. We do not support following syntax for 'do nothing': postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b) do nothing; ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification This limitation is because we do not support unique index on partitioned table. But, in that sense the following snippet of the documentation seems misleading: + will cause an error if the conflict target is specified (see +for more details). That means it's not + possible to specify DO UPDATE as the alternative + action, because it requires the conflict target to be specified. + On the other hand, specifying DO NOTHING as the + alternative action works fine. May be the last sentence can be rephrased as below: "On the other hand, specifying DO NOTHING without target as an alternative action works fine." Other than this patch looks good to me. Regards, Jeevan Ladhe On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote wrote: > Starting a new thread for a patch I posted earlier [1] to handle ON > CONFLICT DO NOTHING when inserting into a partitioned table. It's > intended for PG 11 and so registered in the upcoming CF. > > Summary of the previous discussion and the patch for anyone interested: > > Currently, if an INSERT statement for a partitioned table mentions the ON > CONFLICT clause, we error out immediately. It was implemented that way, > because it was thought that it could not be handled with zero support for > defining indexes on partitioned tables. Peter Geoghegan pointed out [2] > that it's too restrictive a view. > > He pointed out that planner doesn't *always* expect indexes to be present > on the table when ON CONFLICT is specified. They must be present though > if DO UPDATE action is requested, because one would need to also specify > the exact columns on which conflict will be checked and those must covered > by the appropriate indexes. So, if the table is partitioned and DO UPDATE > is specified, lack of indexes will result in an error saying that a > suitable index is absent. DO UPDATE action cannot be supported until we > implement the feature to define indexes on partitioned tables. > > OTOH, the DO NOTHING case should go through the planner without error, > because neither any columns need to be specified nor any indexes need to > be present covering them. So, DO NOTHING on partitioned tables might work > after all. Conflict can only be determined using indexes, which > partitioned tables don't allow, so how? Leaf partitions into which tuples > are ultimately stored can have indexes defined on them, which can be used > to check for the conflict. > > The patch's job is simple: > > - Remove the check in the parser that causes an error the moment the > ON CONFLICT clause is found. > > - Fix leaf partition ResultRelInfo initialization code so that the call > ExecOpenIndices() specifies 'true' for speculative, so that the > information necessary for conflict checking will be initialized in the > leaf partition's ResultRelInfo > > Thanks, > Amit > > [1] > https://www.postgresql.org/message-id/62be3d7a-08f6-5dcb- > f5c8-a5b764ca96df%40lab.ntt.co.jp > > [2] > https://www.postgresql.org/message-id/CAH2-Wzm10T%2B_PWVM4XO5zaknVbAXkOH9- > JW3gRVPm1njLHck_w%40mail.gmail.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
[HACKERS] typo for using "OBJECT_TYPE" for "security label on domain" in "gram.y"
Commit: 3f88672a4e4d8e648d24ccc65937da61c7660854 add "security label on domain" in "gram.y", and set "objtype" to "OBJECT_TYPE". Is this a typo? -- GaoZengqi pgf...@gmail.com zengqi...@gmail.com
