Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Hello Corey, About v18: Patch applies, make check ok, psql tap tests ok. ISTM that contrary to the documentation "\elif something" is not evaluated in all cases, and the resulting code is harder to understand with a nested switch and condition structure: switch default read if switch I wish (this is a personal taste) it could avoid switch-nesting on the very same value. It should also conform to the documentation. If there is no compelling reason for the switch-nesting, I would suggest to move the read_boolean_expression before the swich, to deal with error immediately there, and then to have just one switch. Alternatively if the structure must really be kept, then deal with errors in a first switch, read value *after* switch and deal with other errors there, then start a second switch, and adjust the documentation accordingly? switch errors read if errors // no error switch Also, the %R documentation has single line marker '^' before not executed '@', but it is the reverse in the code. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] case_preservation_and_insensitivity = on
On Fri, Feb 24, 2017 at 11:04 PM, Joel Jacobson wrote: > On Thu, Feb 23, 2017 at 8:04 AM, Robert Haas wrote: >> It doesn't sound like a good solution to me, because there can be SQL >> code inside stored procedures that clients never see. > > In our code base, we use CamelCase in all PL/pgSQL functions, both for > columns and variables, > e.g. SELECT UserID INTO _UserID FROM Users WHERE Username = 'foo'; > > Here, it's not a problem that the column name is e.g. "userid", > since the case-insensitive feature makes it work. > > What type of case problem do you foresee for stored procedures? If we did something on the server-side, stored procedures would be handled just like everything else, so stored procedures would suffer or be unaffected to precisely the same extent as anything else. However, Tom proposed doing the case-remapping on the client side, which would cause the behavior to be one thing for queries submitted directly from the client and something else for queries that appear inside PL code. -- 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 Sat, Feb 25, 2017 at 11:41 PM, Greg Stark wrote: > What I'm concerned about is that silently giving "wrong" answers in > regular queries -- not even ones doing the partition key updates -- is > something the user can't really manage. They have no way to rewrite > the query to avoid the problem if some other user or part of their > system is updating partition keys. They have no way to know the > problem is even occurring. That's a reasonable concern, but it's not like EvalPlanQual works perfectly today and never causes any application-visible inconsistencies that end up breaking things. As the documentation says: Because of the above rules, it is possible for an updating command to see an inconsistent snapshot: it can see the effects of concurrent updating commands on the same rows it is trying to update, but it does not see effects of those commands on other rows in the database. This behavior makes Read Committed mode unsuitable for commands that involve complex search conditions; however, it is just right for simpler cases. Maybe I've just spent too long hanging out with Kevin Grittner, but I've come to view our EvalPlanQual behavior as pretty rickety and unreliable in general. For example, consider the fact that when I spent over a year and approximately 1 gazillion email messages trying to hammer out how join pushdown was going to EPQ rechecks, we discovered that the FDW API wasn't actually handling those correctly for even for scans of single tables, hence commit 5fc4c26db5120bd90348b6ee3101fcddfdf54800. I'm not saying that time and effort wasn't well-spent, but I wonder whether it's necessary to hold partitioned tables to a higher standard than that to which the FDW interface was held for the first 4.5 years of its life. Perhaps it is good for us to do that, but I'm not 100% convinced. It seems like we decide to worry about EvalPlanQual in some cases and not in others more or less arbitrarily. -- 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] GUC for cleanup indexes threshold.
On Sat, Feb 25, 2017 at 3:40 AM, Peter Geoghegan wrote: > On Fri, Feb 24, 2017 at 9:26 AM, Robert Haas wrote: >> I think this thread is pretty short on evidence that would let us make >> a smart decision about what to do here. I see three possibilities. >> The first is that this patch is a good idea whether we do something >> about the issue of half-dead pages or not. The second is that this >> patch is a good idea if we do something about the issue of half-dead >> pages but a bad idea if we don't. The third is that this patch is a >> bad idea whether or not we do anything about the issue of half-dead >> pages. > > Half-dead pages are not really relevant to this discussion, AFAICT. I > think that both you and Simon mean "recyclable" pages. There are > several levels of indirection involved here, to keep the locking very > granular, so it gets tricky to talk about. Thanks for the clarification. I wasn't very clear on what was going on here; that helps. The thing that strikes me based on what you wrote is that our page recycling seems to admit of long delays even as things stand. There's no bound on how much time could pass between one index vacuum and the next, and RecentGlobalXmin could and probably usually will advance past the point that would allow recycling long before the next index vacuum cycle. I don't know whether that strengthens or weakens Simon's argument. On the one hand, you could argue that if we're already doing this on a long delay, making it even longer probably won't hurt much. On the other hand, you could argue that if the current situation is bad, we should at least avoid making it worse. I don't know which of those arguments is correct, if either. Do you have an idea about that, or any ideas for experiments we could try? -- 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] Making clausesel.c Smarter
On Fri, Feb 24, 2017 at 3:30 PM, David Rowley wrote: > It would be good to improve the situation here in the back branches > too, but I'm thinking that the attached might be a little invasive for > that? My experience has been that customers - at least EnterpriseDB customers - do not appreciate plan changes when they upgrade to a new minor release. Most people have production installations that are basically working; if not, they wouldn't be in production. Even if they're getting a good plan only by some happy accident, they're still getting it, and a change can cause a good plan to flop over into a bad plan, which can easily turn into a service outage. The people who had a bad plan and get flipped to a good plan will be happy once they realize what has changed, of course, but that's not really enough to make up from the panicked calls from customers whose stuff falls over when they try to apply the critical security update. I think the basic think you are trying to accomplish is sensible, though. I haven't reviewed the patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : Parallel Merge Join
On Fri, Feb 24, 2017 at 3:54 PM, Amit Kapila wrote:
> I agree in some cases it could be better, but I think benefits are not
> completely clear, so probably we can leave it as of now and if later
> any one comes with a clear use case or can see the benefits of such
> path then we can include it.
TBH, I think Dilip had the right idea here. cheapest_total_inner
isn't anything magical; it's just that there's no reason to use
anything but the cheapest path for a relation when forming a join path
unless that first path lacks some property that you need, like having
the right sortkeys or being parallel-safe. Since there are lots of
join paths that just want to do things in the cheapest way possible,
we identify the cheapest path and hang on to it; when that's not what
we need, we go find the path or paths that have the properties we
want. I'm not sure why this case should be an exception. You could
argue that if the cheapest parallel-safe path is already more
expensive then the parallel join may not pay off, but that's hard to
say; it depends on what happens higher up in the plan tree. That's
why the current code handles partial hash joins this way:
/*
* Normally, given that the joinrel is parallel-safe, the cheapest
* total inner path will also be parallel-safe, but if not, we'll
* have to search cheapest_parameterized_paths for the cheapest
* safe, unparameterized inner path. If doing JOIN_UNIQUE_INNER,
* we can't use any alternative inner path.
*/
if (cheapest_total_inner->parallel_safe)
cheapest_safe_inner = cheapest_total_inner;
else if (save_jointype != JOIN_UNIQUE_INNER)
{
ListCell *lc;
foreach(lc, innerrel->cheapest_parameterized_paths)
{
Path *innerpath = (Path *) lfirst(lc);
if (innerpath->parallel_safe &&
bms_is_empty(PATH_REQ_OUTER(innerpath)))
{
cheapest_safe_inner = innerpath;
break;
}
}
}
I would tend to think this case ought to be handled in a similar way.
And if not, then we ought to go change the hash join logic too so that
they match.
--
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] Documentation improvements for partitioning
On Sun, Feb 26, 2017 at 4:31 AM, Bruce Momjian wrote: > I think you are right. I was only guessing on a possible cause of > Simon's reaction since I had the same reaction. When traveling, it is > hard to get excited about reading a 100+ post thread that has reached a > conclusion. I found Simon's summary of the 4 sub-features to be > helpful. OK, no problem. Basically, I think it's a bad plan to redesign this - or add any large amount of incremental change to what's already been done - at this point in the release cycle. Unless we're prepared to rip it all back out, we've got to ship more or less what we have and improve it later. I always viewed the mission of this patch as to set the stage for future improvements in this area, not to solve all of the problems by itself. I'm sorry if anyone was under a contrary impression, and I'm also sorry that the discussion seems to have left some people behind, but I did try my best. -- 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: Covering + unique indexes.
On Thu, Feb 16, 2017 at 6:43 PM, Anastasia Lubennikova
wrote:
> 14.02.2017 15:46, Amit Kapila:
>
>
>> 4.
>> + IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P
>> INCLUDE
>>INCLUDING INCREMENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
>>INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
>>INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
>> @@ -3431,17 +3433,18 @@ ConstraintElem:
>>n->initially_valid = !n->skip_validation;
>>$$ = (Node *)n;
>>}
>> - | UNIQUE '(' columnList ')' opt_definition OptConsTableSpace
>> + | UNIQUE '(' columnList ')' opt_c_including opt_definition
>> OptConsTableSpace
>>
>> If we want to use INCLUDE in syntax, then it might be better to keep
>> the naming reflect the same. For ex. instead of opt_c_including we
>> should use opt_c_include.
>>
>
>
> Thank you for this suggestion. I've just wrote the code looking at examples
> around,
> but optincluding and optcincluding clauses seem to be redundant.
> I've cleaned up the code.
>
I think you have cleaned only in gram.y as I could see the references
to 'including' in other parts of code. For ex, see below code:
@@ -2667,6 +2667,7 @@ _copyConstraint(const Constraint *from)
COPY_NODE_FIELD(raw_expr);
COPY_STRING_FIELD(cooked_expr);
COPY_NODE_FIELD(keys);
+ COPY_NODE_FIELD(including);
COPY_NODE_FIELD(exclusions);
COPY_NODE_FIELD(options);
COPY_STRING_FIELD(indexname);
@@ -3187,6 +3188,7 @@ _copyIndexStmt(const IndexStmt *from)
COPY_STRING_FIELD(accessMethod);
COPY_STRING_FIELD(tableSpace);
COPY_NODE_FIELD(indexParams);
+ COPY_NODE_FIELD(indexIncludingParams);
@@ -425,6 +425,13 @@ ConstructTupleDescriptor(Relation heapRelation,
/*
+ * Code below is concerned to the opclasses which are not used
+ * with the included columns.
+ */
+ if (i >= indexInfo->ii_NumIndexKeyAttrs)
+ continue;
+
There seems to be code below the above check which is not directly
related to opclasses, so not sure if you have missed that or is there
any other reason to ignore that. I am referring to following code in
the same function after the above check:
/*
* If a key type different from the heap value is specified, update
*
the type-related fields in the index tupdesc.
*/
if (OidIsValid(keyType) &&
keyType != to->atttypid)
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
> > Typo "unterminted". > Fixed. > The \if within the \if false branch is not tallied properly? Am I missing > something? > Nope, you found a bug. FIxed. Test-case added. > I changed the paragraph to >> > >Lines within false branches are parsed normally, however, any >> completed >>queries are not sent to the server, and any completed commands >> other >>than conditionals (\if, >> \elif, >>\else, \endif) are ignored. >> > > I'm not sure about the ", however, " commas, but I'm sure that I do not > know English punctuation rules:-) > Re-worded it again for shorter sentences. Re-mentioned that conditionals are still checked for proper nesting. * Changed comments to reflect that \if always evalutes even in a false branch * Changed \elif to first check if the command is in a proper \if block before evaluating the expression. The invalid boolean message would mask the bigger problem. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae58708..27824d9 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -2035,6 +2035,83 @@ hello 10 + +\if expr +\elif expr +\else +\endif + + +This group of commands implements nestable conditional blocks, like +this: + + +-- set ON_ERROR_STOP in case the variables are not valid boolean expressions +\set ON_ERROR_STOP on +-- check for the existence of two separate records in the database and store +-- the results in separate psql variables +SELECT +EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer, +EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee +\gset +\if :is_customer +SELECT * FROM customer WHERE customer_id = 123; +\elif :is_employee +\echo 'is not a customer but is an employee' +SELECT * FROM employee WHERE employee_id = 456; +\else +\if yes +\echo 'not a customer or employee' +\else +\echo 'this should never print' +\endif +\endif + + +Conditional blocks must begin with a \if and end +with an \endif, and the pairs must be found in +the same source file. If an EOF is reached on the main file or an +\include-ed file before all local +\if-\endif are matched, then +psql will raise an error. + + +A conditional block can have any number of +\elif clauses, which may optionally be followed by a +single \else clause. + + +The \if and \elif commands +read the rest of the line and evaluate it as a boolean expression. +Currently, expressions are limited to a single unquoted string +which are evaluated like other on/off options, so the valid values +are any unambiguous case insensitive matches for one of: +true, false, 1, +0, on, off, +yes, no. So +t, T, and tR +will all match true. + +Expressions that do not properly evaluate to true or false will +generate an error and cause the \if or +\elif command to fail. Because that behavior may +change branching context in undesirable ways (executing code which +was intended to be skipped, causing \elif, +\else, and \endif commands to +pair with the wrong \if, etc), it is +recommended that scripts which use conditionals also set +ON_ERROR_STOP. + + +Lines within false branches are parsed normally for query/command +completion, but any queries are not sent to the server, and any +commands other than conditionals (\if, +\elif, \else, +\endif) are ignored. Conditional commands are +still checked for valid nesting. + + + \ir or \include_relative filename @@ -3703,7 +3780,8 @@ testdb=> INSERT INTO my_table VALUES (:'content'); In prompt 1 normally =, -but ^ if in single-line mode, +but ^ if in single-line mode, or +@ if the session is in a false conditional block, or ! if the session is disconnected from the database (which can happen if \connect fails). In prompt 2 %R is replaced by a character that diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index c53733f..1492b66 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq -OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \ +OBJS= command.o common.o conditional.o help.o input.o stringutils.o mainloop.o copy.o \ startup.o prompt.o variables.o large_obj.o describe.o \ crosstabview.o tab-complete.o \ sql_hel
Re: [HACKERS] Enabling parallelism for queries coming from SQL or other PL functions
On Sat, Feb 25, 2017 at 9:47 PM, Dilip Kumar wrote: > On Sat, Feb 25, 2017 at 5:12 PM, Amit Kapila wrote: >> Sure, but that should only happen if the function is *not* declared as >> parallel safe (aka in parallel safe functions, we should not generate >> parallel plans). > > So basically we want to put a restriction that parallel-safe function > can not use the parallel query? This will work but it seems too > restrictive to me. Because by marking function parallel safe we enable > it to be used with the outer parallel query that is fine. But, that > should not restrict the function from using the parallel query if it's > used with the other outer query which is not having the parallel > plan(or function is executed directly). > I think if the user is explicitly marking a function as parallel-safe, then it doesn't make much sense to allow parallel query in such functions as it won't be feasible for the planner (or at least it will be quite expensive) to detect the same. By the way, if the user has any such expectation from a function, then he can mark the function as parallel-restricted or parallel-unsafe. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical replication existing data copy
On 25/02/17 09:40, Erik Rijkers wrote: > On 2017-02-25 00:40, Petr Jelinek wrote: > >> 0001-Use-asynchronous-connect-API-in-libpqwalreceiver.patch >> 0002-Fix-after-trigger-execution-in-logical-replication.patch >> 0003-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION.patch >> snapbuild-v3-0001-Reserve-global-xmin-for-create-slot-snasphot-export.patch >> >> snapbuild-v3-0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch >> >> snapbuild-v3-0003-Fix-xl_running_xacts-usage-in-snapshot-builder.patch >> snapbuild-v3-0004-Skip-unnecessary-snapshot-builds.patch >> 0001-Logical-replication-support-for-initial-data-copy-v6.patch > > Here are some results. There is improvement although it's not an > unqualified success. > Yep, I found yet another bug in snapbuild (see the relevant thread). I hope with the updated patches from there (now 5 of them) it should finally work correctly. > I want to repeat what I said a few emails back: problems seem to > disappear when a short wait state is introduced (directly after the > 'alter subscription sub1 enable' line) to give the logrep machinery time > to 'settle'. It makes one think of a timing error somewhere (now don't > ask me where..). I think that's because if you sleep, the COPY finishes in meantime and so there is no concurrency with pgbench when the initial snapshot is being taken so that's not solution for production. > > (By the way, no hanged sessions so far, so that's good) > Great. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] snapbuild woes
On 24/02/17 22:56, Petr Jelinek wrote: > On 22/02/17 03:05, Petr Jelinek wrote: >> On 13/12/16 00:38, Petr Jelinek wrote: >>> On 12/12/16 23:33, Andres Freund wrote: On 2016-12-12 23:27:30 +0100, Petr Jelinek wrote: > On 12/12/16 22:42, Andres Freund wrote: >> Hi, >> >> On 2016-12-10 23:10:19 +0100, Petr Jelinek wrote: >>> Hi, >>> First one is outright bug, which has to do with how we track running >>> transactions. What snapbuild basically does while doing initial snapshot >>> is read the xl_running_xacts record, store the list of running txes and >>> then wait until they all finish. The problem with this is that >>> xl_running_xacts does not ensure that it only logs transactions that are >>> actually still running (to avoid locking PGPROC) so there might be xids >>> in xl_running_xacts that already committed before it was logged. >> >> I don't think that's actually true? Notice how LogStandbySnapshot() >> only releases the lock *after* the LogCurrentRunningXacts() iff >> wal_level >= WAL_LEVEL_LOGICAL. So the explanation for the problem you >> observed must actually be a bit more complex :( >> > > Hmm, interesting, I did see the transaction commit in the WAL before the > xl_running_xacts that contained the xid as running. I only seen it on > production system though, didn't really manage to easily reproduce it > locally. I suspect the reason for that is that RecordTransactionCommit() doesn't conflict with ProcArrayLock in the first place - only ProcArrayEndTransaction() does. So they're still running in the PGPROC sense, just not the crash-recovery sense... >>> >>> That looks like reasonable explanation. BTW I realized my patch needs >>> bit more work, currently it will break the actual snapshot as it behaves >>> same as if the xl_running_xacts was empty which is not correct AFAICS. >>> >> >> Hi, >> >> I got to work on this again. Unfortunately I haven't found solution that >> I would be very happy with. What I did is in case we read >> xl_running_xacts which has all transactions we track finished, we start >> tracking from that new xl_running_xacts again with the difference that >> we clean up the running transactions based on previously seen committed >> ones. That means that on busy server we may wait for multiple >> xl_running_xacts rather than just one, but at least we have chance to >> finish unlike with current coding which basically waits for empty >> xl_running_xacts. I also removed the additional locking for logical >> wal_level in LogStandbySnapshot() since it does not work. > > Not hearing any opposition to this idea so I decided to polish this and > also optimize it a bit. > > That being said, thanks to testing from Erik Rijkers I've identified one > more bug in how we do the initial snapshot. Apparently we don't reserve > the global xmin when we start building the initial exported snapshot for > a slot (we only reserver catalog_xmin which is fine for logical decoding > but not for the exported snapshot) so the VACUUM and heap pruning will > happily delete old versions of rows that are still needed by anybody > trying to use that exported snapshot. > Aaand I found one more bug in snapbuild. Apparently we don't protect the snapshot builder xmin from going backwards which can yet again result in corrupted exported snapshot. Summary of attached patches: 0001 - Fixes the above mentioned global xmin tracking issues. Needs to be backported all the way to 9.4 0002 - Removes use of the logical decoding saved snapshots for initial exported snapshot since those snapshots only work for catalogs and not user data. Also needs to be backported all the way to 9.4. 0003 - Makes sure snapshot builder xmin is not moved backwards by xl_running_xacts (which can otherwise happen during initial snapshot building). Also should be backported to 9.4. 0004 - Changes handling of the xl_running_xacts in initial snapshot build to what I wrote above and removes the extra locking from LogStandbySnapshot introduced by logical decoding. 0005 - Improves performance of initial snapshot building by skipping catalog snapshot build for transactions that don't do catalog changes. I did some improvements to the other patches as well so they are not the same as in previous post, hence the version bump. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services From dfa0b07639059675704a35f2e63be4934f97c3a3 Mon Sep 17 00:00:00 2001 From: Petr Jelinek Date: Fri, 24 Feb 2017 21:39:03 +0100 Subject: [PATCH 1/5] Reserve global xmin for create slot snasphot export Otherwise the VACUUM or pruning might remove tuples still needed by the exported snapshot. --- src/backend/replication/logical/logical.c | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/backend/replication/logical/logical.c b/src
Re: [HACKERS] bytea_output output of base64
On Sat, Feb 25, 2017 at 06:49:01PM -0500, Peter Eisentraut wrote: > On 2/23/17 17:55, Bruce Momjian wrote: > > On Thu, Feb 23, 2017 at 04:08:58PM -0500, Tom Lane wrote: > >> Bruce Momjian writes: > >>> Is there a reason we don't support base64 as a bytea_output output > >>> option, except that no one has implemented it? > >> > >> How about "we already have one too many bytea output formats"? > >> I don't think forcing code to try to support still another one > >> is a great thing ... especially not if it couldn't be reliably > >> distinguished from the hex format. > > > > Is there a reason we chose hex over base64? > > The reason we changed from the old format to hex was for performance. > We didn't consider base64 at the time, but hex would probably still have > been faster. OK, good to know. I was just asking. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bytea_output output of base64
On 2/23/17 17:55, Bruce Momjian wrote: > On Thu, Feb 23, 2017 at 04:08:58PM -0500, Tom Lane wrote: >> Bruce Momjian writes: >>> Is there a reason we don't support base64 as a bytea_output output >>> option, except that no one has implemented it? >> >> How about "we already have one too many bytea output formats"? >> I don't think forcing code to try to support still another one >> is a great thing ... especially not if it couldn't be reliably >> distinguished from the hex format. > > Is there a reason we chose hex over base64? The reason we changed from the old format to hex was for performance. We didn't consider base64 at the time, but hex would probably still have been faster. -- 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] Automatic cleanup of oldest WAL segments with pg_receivexlog
On Sun, Feb 26, 2017 at 12:41 AM, Magnus Hagander wrote: > On Feb 25, 2017 15:00, "Michael Paquier" wrote: > > On Sat, Feb 25, 2017 at 10:32 PM, Magnus Hagander > wrote: >> Oh, I definitely think such a command should be able to take a placeholder >> like %f telling which segment it has just processed. In fact, I'd consider >> it one of the most important features of it :) > > I cannot think about any other meaningful variables, do you? > > > Not offhand. But one thing that could go to the question of parameter name - > what if we finish something that's not a segment. During a time line switch > for example, we also get other files don't we? We probably want to trigger > at least some command in that case - either with an argument or by a > different parameter? To be consistent with archive_command and restore_command I'd rather not do that. The command called can decide by itself what to do by looking at the shape of the argument string. -- 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] Documentation improvements for partitioning
On Wed, Feb 22, 2017 at 07:44:41AM +0530, Robert Haas wrote: > On Tue, Feb 21, 2017 at 2:03 AM, Bruce Momjian wrote: > > I have to admit my reaction was similar to Simon's, meaning that the > > lack of docs is a problem, and that the limitations are kind of a > > surprise, and I wonder what other surprises there are. > > Did you read my message upthread pointing out that the initial commit > contained hundreds of lines of documentation? I agree that it would > be bad if table partitioning got committed with no documentation, but > that did not happen. > > > I am thinking this is a result of small teams, often from the same > > company, working on a features in isolation and then making them public. > > It is often not clear what decisions were made and why. > > That also did not happen, or at least certainly not with this patch. > All of the discussion was public and on the mailing list. I never > communicated with Amit Langote off-list about this work, except > shortly before I committed his patches I added him on Skype and gave > him a heads up that I was planning to do so real soon. At no time > have the two of us worked for the same company. Also, the patch had 7 > other reviewers credited in the commit messages spread across, I > think, 4 different companies. I think you are right. I was only guessing on a possible cause of Simon's reaction since I had the same reaction. When traveling, it is hard to get excited about reading a 100+ post thread that has reached a conclusion. I found Simon's summary of the 4 sub-features to be helpful. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Should logtape.c blocks be of type long?
logtape.c stores block numbers on disk. These block numbers are
represented in memory as being of type long. This is why BufFile
clients that use the block-based interface (currently limited to
logtape.c and gistbuildbuffers.c) must live with a specific
limitation, as noted within buffile.c:
/*
* BufFileSeekBlock --- block-oriented seek
*
* Performs absolute seek to the start of the n'th BLCKSZ-sized block of
* the file. Note that users of this interface will fail if their files
* exceed BLCKSZ * LONG_MAX bytes, but that is quite a lot; we don't work
* with tables bigger than that, either...
*
* Result is 0 if OK, EOF if not. Logical position is not moved if an
* impossible seek is attempted.
*/
int
BufFileSeekBlock(BufFile *file, long blknum)
{
...
}
That restriction is fine on 64-bit Linux, where it is actually true
that "we don't work with tables that big either", but on Win64 long is
still only 4 bytes. 32-bit systems are similarly affected. Of course,
MaxBlockNumber is 0xFFFE, whereas LONG_MAX is only 0x7FFF on
affected systems.
Is it worth changing this interface to use int64, which is already
defined to be a "long int" on most real-world installations anyway?
Though it hardly matters, this would have some cost: logtape.c temp
files would have a higher storage footprint on win64 (i.e., the same
overhead as elsewhere).
I tend to be suspicious of use of the type "long" in general, because
in general one should assume that it is no wider than "int". This
calls into question why any code that uses "long" didn't just use
"int", at least in my mind.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 23 February 2017 at 16:33, Simon Riggs wrote: > I'll be happy to review Patch looks OK so far, but fails on a partition that has partitions, probably because of the way we test relkind in the call to StoreCatalogInheritance1(). Please add a test for that so we can check automatically. Thanks very much. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] I propose killing PL/Tcl's "modules" infrastructure
On 02/25/2017 02:21 PM, Tom Lane wrote: > Andrew Dunstan writes: >> On 02/25/2017 01:44 PM, Tom Lane wrote: >>> Yeah, the only part that's even a bit hard to replicate in userland is >>> initializing the autoloading mechanism in each session. It would be >>> cleaner to provide a feature similar to what you describe that could >>> be used for that purpose as well as others. However, where does the >>> "parameterless function" come from? Is it a regular PLv8 (or for this >>> purpose PL/Tcl) function expected to be present in pg_proc? >> Yes, it's a regular PLv8 function. > OK ... how do you handle security considerations? Can the GUC be set > at any time/by anybody? What determines whether you have permissions > to call the particular function? > > It can be set by anyone, IIRC. Maybe it should be SUSET only, I don't know. It's executed as the session owner. Execute permission on the function are determined the same way as for any function. It's an ordinary function call. The only difference is in how the call gets triggered. cheers andrew -- Andrew Dunstanhttps://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] Proposal for changes to recovery.conf API
On 25 February 2017 at 13:58, Michael Paquier wrote: > - trigger_file is removed. > FWIW, my only complain is about the removal of trigger_file, this is > useful to detect a trigger file on a different partition that PGDATA! > Keeping it costs also nothing.. Sorry, that is just an error of implementation, not intention. I had it on my list to keep, at your request. New version coming up. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] I propose killing PL/Tcl's "modules" infrastructure
Andrew Dunstan writes: > On 02/25/2017 01:44 PM, Tom Lane wrote: >> Yeah, the only part that's even a bit hard to replicate in userland is >> initializing the autoloading mechanism in each session. It would be >> cleaner to provide a feature similar to what you describe that could >> be used for that purpose as well as others. However, where does the >> "parameterless function" come from? Is it a regular PLv8 (or for this >> purpose PL/Tcl) function expected to be present in pg_proc? > Yes, it's a regular PLv8 function. OK ... how do you handle security considerations? Can the GUC be set at any time/by anybody? What determines whether you have permissions to call the particular function? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] UPDATE of partition key
On Sat, Feb 25, 2017 at 11:11 AM, Greg Stark wrote: > On 24 February 2017 at 14:57, David G. Johnston > wrote: > > I dislike an error. I'd say that making partition "just work" here is > > material for another patch. In this one an update of the partition key > can > > be documented as shorthand for delete-returning-insert with all the > > limitations that go with that. If someone acceptably solves the ctid > > following logic later it can be committed - I'm assuming there would be > no > > complaints to making things just work in a case where they only sorta > > worked. > > Personally I don't think there's any hope that there will ever be > cross-table ctids links. Maybe one day there will be a major new table > storage format with very different capabilities than today but in the > current architecture it seems like an impossible leap. > How about making it work without a physical token dynamic? For instance, let the server recognize the serialization error but instead of returning it to the client the server itself tries again. > I would expect everyone to come to terms with the basic idea that > partition key updates are always going to be a corner case. The user > defined the partition key and the docs should carefully explain to > them the impact of that definition. As long as that explanation gives > them something they can work with and manage the consequences of > that's going to be fine. > > What I'm concerned about is that silently giving "wrong" answers in > regular queries -- not even ones doing the partition key updates -- is > something the user can't really manage. They have no way to rewrite > the query to avoid the problem if some other user or part of their > system is updating partition keys. They have no way to know the > problem is even occurring. > > Just to spell it out -- it's not just "no-op updates" where the user > sees 0 records updated. If I update all records where > username='stark', perhaps to set the "user banned" flag and get back > "9 records updated" and later find out that I missed a record because > someone changed the department_id while my query was running -- how > would I even know? How could I possibly rewrite my query to avoid > that? > But my point is that this isn't a regression from current behavior. If I deleted one of those starks and re-inserted them with a different department_id that brand new record wouldn't be banned. In short, my take on this patch is that it is a performance optimization. Making the UPDATE command actually work as part of its implementation detail is a happy byproduct. >From the POV of an external observer it doesn't have to matter whether the update or delete-insert SQL was used. It would be nice if the UPDATE version could keep logical identity maintained but that is a feature enhancement. Failing if the other session used the UPDATE SQL isn't wrong; and I'm not against it, I just don't believe that it is better than maintaining the status quo semantics. That said my concurrency-fu is not that strong and I don't really have a practical reason to prefer one over the other - thus I fall back on maintaining internal consistency. IIUC it is already possible, for those who care to do so, to get a serialization failure in this scenario by upgrading isolation to repeatable read. David J.
Re: [HACKERS] I propose killing PL/Tcl's "modules" infrastructure
On 02/25/2017 01:44 PM, Tom Lane wrote:
> Andrew Dunstan writes:
>> On 02/25/2017 01:14 PM, Tom Lane wrote:
>>> Now, we could try to fix this bug, and add the regression test coverage
>>> that the code clearly lacks, and upgrade the documentation about it from
>>> its currently very sad state. But I think the right answer is just to
>>> remove the feature altogether. It's evidently not being used, and it's
>>> kind of insecure by design, and it would not be that hard for someone
>>> to provide equivalent functionality entirely in userland if they really
>>> wanted it.
>> In PLv8 we added a parameter plv8.start_proc that names a parameterless
>> function that's executed when plv8 is first called in each session. It
>> can be used quite easily to implement something like a modules
>> infrastructure - in fact I have used it to good effect for exactly that.
>> Maybe something similar for pltcl would be a good thing.
> Yeah, the only part that's even a bit hard to replicate in userland is
> initializing the autoloading mechanism in each session. It would be
> cleaner to provide a feature similar to what you describe that could
> be used for that purpose as well as others. However, where does the
> "parameterless function" come from? Is it a regular PLv8 (or for this
> purpose PL/Tcl) function expected to be present in pg_proc?
>
>
Yes, it's a regular PLv8 function.Here's an example. It presupposes that
there is a table called plv8_modules (modname text, code text,
load_on_start boolean).
CREATE OR REPLACE FUNCTION public.plv8_startup()
RETURNS void
LANGUAGE plv8
AS $function$
if (typeof plv8_loaded_modules == 'undefined')
plv8_loaded_modules = {};
load_module = function(modname)
{
if (plv8_loaded_modules[modname])
return;
var rows = plv8.execute("SELECT code from plv8_modules " +
" where modname = $1", [modname]);
for (var r = 0; r < rows.length; r++)
{
var code = rows[r].code;
eval("(function() { " + code + "})")();
// plv8.elog(NOTICE,"loaded module " + modname);
plv8_loaded_modules[modname] = 1;
}
};
reload_module = function(modname)
{
var rows = plv8.execute("SELECT code from plv8_modules " +
" where modname = $1", [modname]);
for (var r = 0; r < rows.length; r++)
{
var code = rows[r].code;
eval("(function() { " + code + "})")();
// plv8.elog(NOTICE,"loaded module " + modname);
plv8_loaded_modules[modname] = 1;
}
};
var rows = plv8.execute("SELECT modname, code from plv8_modules
where load_on_start");
for (var r = 0; r < rows.length; r++)
{
var modname = rows[r].modname;
if (plv8_loaded_modules[modname])
continue;
var code = rows[r].code;
eval("(function() { " + code + "})")();
plv8_loaded_modules[modname] = 1;
// plv8.elog(NOTICE,"loaded module " + modname);
};
$function$;
cheers
andrew
--
Andrew Dunstanhttps://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] I propose killing PL/Tcl's "modules" infrastructure
"Joshua D. Drake" writes: > I don't see a reason to keep pl/tcl in core at all so ripping out the > functionality seems the least disruptive and perhaps even a deprecation > of the PL (at least from a core perspective) in v10. I'm not in any hurry to remove or deprecate PL/Tcl as a whole. It's gotten more love in the past year than it got in the previous ten, so evidently there are still people out there who use it. But they don't seem to be using this particular part of it. 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] I propose killing PL/Tcl's "modules" infrastructure
Andrew Dunstan writes: > On 02/25/2017 01:14 PM, Tom Lane wrote: >> Now, we could try to fix this bug, and add the regression test coverage >> that the code clearly lacks, and upgrade the documentation about it from >> its currently very sad state. But I think the right answer is just to >> remove the feature altogether. It's evidently not being used, and it's >> kind of insecure by design, and it would not be that hard for someone >> to provide equivalent functionality entirely in userland if they really >> wanted it. > In PLv8 we added a parameter plv8.start_proc that names a parameterless > function that's executed when plv8 is first called in each session. It > can be used quite easily to implement something like a modules > infrastructure - in fact I have used it to good effect for exactly that. > Maybe something similar for pltcl would be a good thing. Yeah, the only part that's even a bit hard to replicate in userland is initializing the autoloading mechanism in each session. It would be cleaner to provide a feature similar to what you describe that could be used for that purpose as well as others. However, where does the "parameterless function" come from? Is it a regular PLv8 (or for this purpose PL/Tcl) function expected to be present in pg_proc? 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] Should `pg_upgrade --check` check relation filenodes are present?
On Wed, Feb 1, 2017 at 10:57:01AM +1300, Craig de Stigter wrote:
> Hi list
>
> We attempted to pg_upgrade a database on a replication slave, and got the
> error:
>
>
> error while creating link for relation "." ("/var/
> lib/postgresql-ext/PG_9.2_201204301/19171/141610397" to "/var/lib/
> postgresql-ext/PG_9.5_201510051/16401/9911696"): No such file or
> directory
>
>
>
>
> The missing table turned out to be an unlogged table, and the data file for it
> was not present on the slave machine. That's reasonable. In our case we are
> able to start over from a snapshot and drop all the unlogged tables before
> trying again.
>
> However, this problem was not caught by the `--check` command. I'm looking at
> the source code and it appears that pg_upgrade does not attempt to verify
> relation filenodes actually exist before proceeding, whether using --check or
> not.
>
> Should it? I assume the reasoning is because it would take a long time and
> perhaps the benefit of doing so would be minimal?
I think pg_upgrade needs to be improved in this area, but I am not sure
how yet. Clearly the check should detect this or the upgrade should
succeed.
First, you are not using the standby upgrade instructions in step 10
here, right?
https://www.postgresql.org/docs/9.6/static/pgupgrade.html
I assume you don't want this standby to rejoin the primary, you just
want to upgrade it.
Second, I thought unlogged tables had empty files on the standby, not
_missing_ files. Is that correct? Should pg_upgrade just allow missing
unlogged table files? I don't see any way to detect we are running on a
standby since the server is in write mode to run pg_upgrade.
I can develop a patch once I have answers to these questions.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] btree_gin and btree_gist for enums
On 02/25/2017 01:34 PM, Tom Lane wrote: > Andrew Dunstan writes: >> On 02/25/2017 12:04 PM, Tom Lane wrote: >>> I think it'd be better to leave DirectFunctionCallN alone and just invent >>> a small number of CallerFInfoFunctionCallN support functions (maybe N=1 >>> and N=2 would be enough, at least for now). >> See attached. > Yeah, I like this better, except that instead of > > + * The callee should not look at anything except the fn_mcxt and fn_extra. > + * Anything else is likely to be bogus. > > maybe > > + * It's recommended that the callee only use the fn_extra and fn_mcxt > + * fields, as other fields will typically describe the calling function > + * not the callee. Conversely, the calling function should not have > + * used fn_extra, unless its use is known compatible with the callee's. > > OK, Works for me. Thanks. cheers andrew -- Andrew Dunstanhttps://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] I propose killing PL/Tcl's "modules" infrastructure
On 02/25/2017 10:14 AM, Tom Lane wrote: Now, we could try to fix this bug, and add the regression test coverage that the code clearly lacks, and upgrade the documentation about it from its currently very sad state. But I think the right answer is just to remove the feature altogether. It's evidently not being used, and it's kind of insecure by design, and it would not be that hard for someone to provide equivalent functionality entirely in userland if they really wanted it. I don't see a reason to keep pl/tcl in core at all so ripping out the functionality seems the least disruptive and perhaps even a deprecation of the PL (at least from a core perspective) in v10. JD -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. Unless otherwise stated, opinions are my own. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] I propose killing PL/Tcl's "modules" infrastructure
On 02/25/2017 01:14 PM, Tom Lane wrote: > Over in > https://www.postgresql.org/message-id/alpine.DEB.2.11.1702251701030.3920@Sandal.Woodpecker > it's pointed out that pltcl_loadmod was never updated for the switch > to standard_conforming_strings (and the patch proposed there doesn't > begin to cover all the places that would need fixed for that). > > This means that the "modules" functionality is entirely broken in any > installation that's got standard_conforming_strings turned on, which has > been the default since 9.1 and was possible long before that. > > The fact that nobody has noticed seems to me to be clear proof that no one > is using this feature in the field. > > Now, we could try to fix this bug, and add the regression test coverage > that the code clearly lacks, and upgrade the documentation about it from > its currently very sad state. But I think the right answer is just to > remove the feature altogether. It's evidently not being used, and it's > kind of insecure by design, and it would not be that hard for someone > to provide equivalent functionality entirely in userland if they really > wanted it. > > Comments? > > In PLv8 we added a parameter plv8.start_proc that names a parameterless function that's executed when plv8 is first called in each session. It can be used quite easily to implement something like a modules infrastructure - in fact I have used it to good effect for exactly that. Maybe something similar for pltcl would be a good thing. cheers andrew -- Andrew Dunstanhttps://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] btree_gin and btree_gist for enums
Andrew Dunstan writes: > On 02/25/2017 12:04 PM, Tom Lane wrote: >> I think it'd be better to leave DirectFunctionCallN alone and just invent >> a small number of CallerFInfoFunctionCallN support functions (maybe N=1 >> and N=2 would be enough, at least for now). > See attached. Yeah, I like this better, except that instead of + * The callee should not look at anything except the fn_mcxt and fn_extra. + * Anything else is likely to be bogus. maybe + * It's recommended that the callee only use the fn_extra and fn_mcxt + * fields, as other fields will typically describe the calling function + * not the callee. Conversely, the calling function should not have + * used fn_extra, unless its use is known compatible with the callee's. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PL/Python: Add cursor and execute methods to plan object
Something that has been bothering me in PL/Python for a long time is the
non-object-oriented way in which plans are prepared and executed:
plan = plpy.prepare(...)
res = plpy.execute(plan, ...)
where plpy.execute() takes either a plan or a query string.
I think a better style would be
plan = plpy.prepare(...)
res = plan.execute(...)
so that the "plan" is more like a statement handle that one finds in
other APIs.
This ended up being very easy to implement, so I'm proposing to allow
this new syntax as an alternative.
I came across this again as I was developing the background sessions API
for PL/Python. So I'm also wondering here which style people prefer so
I can implement it there.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9dccf70110d9d5818318c651c2662f2b8f86b2bc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 25 Feb 2017 08:42:25 -0500
Subject: [PATCH] PL/Python: Add cursor and execute methods to plan object
Instead of
plan = plpy.prepare(...)
res = plpy.execute(plan, ...)
you can now write
plan = plpy.prepare(...)
res = plan.execute(...)
or even
res = plpy.prepare(...).execute(...)
and similarly for the cursor() method.
This is more in object oriented style, and makes the hybrid nature of
the existing execute() function less confusing.
---
doc/src/sgml/plpython.sgml| 14 --
src/pl/plpython/expected/plpython_spi.out | 19 ---
src/pl/plpython/plpy_cursorobject.c | 3 +--
src/pl/plpython/plpy_cursorobject.h | 1 +
src/pl/plpython/plpy_planobject.c | 31 +++
src/pl/plpython/plpy_spi.c| 3 +--
src/pl/plpython/plpy_spi.h| 1 +
src/pl/plpython/sql/plpython_spi.sql | 18 --
8 files changed, 79 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
index 46397781be..6888ce1ae3 100644
--- a/doc/src/sgml/plpython.sgml
+++ b/doc/src/sgml/plpython.sgml
@@ -1048,6 +1048,14 @@ Database Access Functions
+ Alternatively, you can call the execute method on
+ the plan object:
+
+rv = plan.execute(["name"], 5)
+
+
+
+
Query parameters and result row fields are converted between PostgreSQL
and Python data types as described in .
@@ -1082,7 +1090,9 @@ Database Access Functions
as plpy.execute (except for the row limit) and returns
a cursor object, which allows you to process large result sets in smaller
chunks. As with plpy.execute, either a query string
- or a plan object along with a list of arguments can be used.
+ or a plan object along with a list of arguments can be used, or
+ the cursor function can be called as a method of
+ the plan object.
@@ -1126,7 +1136,7 @@ Database Access Functions
CREATE FUNCTION count_odd_prepared() RETURNS integer AS $$
odd = 0
plan = plpy.prepare("select num from largetable where num % $1 <> 0", ["integer"])
-rows = list(plpy.cursor(plan, [2]))
+rows = list(plpy.cursor(plan, [2])) # or: = list(plan.cursor([2]))
return len(rows)
$$ LANGUAGE plpythonu;
diff --git a/src/pl/plpython/expected/plpython_spi.out b/src/pl/plpython/expected/plpython_spi.out
index 0d78ca1de4..e54dca9e2e 100644
--- a/src/pl/plpython/expected/plpython_spi.out
+++ b/src/pl/plpython/expected/plpython_spi.out
@@ -31,6 +31,19 @@ except Exception, ex:
return None
'
LANGUAGE plpythonu;
+CREATE FUNCTION spi_prepared_plan_test_two(a text) RETURNS text
+ AS
+'if "myplan" not in SD:
+ q = "SELECT count(*) FROM users WHERE lname = $1"
+ SD["myplan"] = plpy.prepare(q, [ "text" ])
+try:
+ rv = SD["myplan"].execute([a])
+ return "there are " + str(rv[0]["count"]) + " " + str(a) + "s"
+except Exception, ex:
+ plpy.error(str(ex))
+return None
+'
+ LANGUAGE plpythonu;
CREATE FUNCTION spi_prepared_plan_test_nested(a text) RETURNS text
AS
'if "myplan" not in SD:
@@ -80,8 +93,8 @@ select spi_prepared_plan_test_one('doe');
there are 3 does
(1 row)
-select spi_prepared_plan_test_one('smith');
- spi_prepared_plan_test_one
+select spi_prepared_plan_test_two('smith');
+ spi_prepared_plan_test_two
there are 1 smiths
(1 row)
@@ -372,7 +385,7 @@ plan = plpy.prepare(
["text"])
for row in plpy.cursor(plan, ["w"]):
yield row['fname']
-for row in plpy.cursor(plan, ["j"]):
+for row in plan.cursor(["j"]):
yield row['fname']
$$ LANGUAGE plpythonu;
CREATE FUNCTION cursor_plan_wrong_args() RETURNS SETOF text AS $$
diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 7bb8992148..18e689f141 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -25,7 +25,6 @@
static PyObject *PLy_cursor_query(const char *query);
-static PyObject *PLy_cursor_
[HACKERS] I propose killing PL/Tcl's "modules" infrastructure
Over in https://www.postgresql.org/message-id/alpine.DEB.2.11.1702251701030.3920@Sandal.Woodpecker it's pointed out that pltcl_loadmod was never updated for the switch to standard_conforming_strings (and the patch proposed there doesn't begin to cover all the places that would need fixed for that). This means that the "modules" functionality is entirely broken in any installation that's got standard_conforming_strings turned on, which has been the default since 9.1 and was possible long before that. The fact that nobody has noticed seems to me to be clear proof that no one is using this feature in the field. Now, we could try to fix this bug, and add the regression test coverage that the code clearly lacks, and upgrade the documentation about it from its currently very sad state. But I think the right answer is just to remove the feature altogether. It's evidently not being used, and it's kind of insecure by design, and it would not be that hard for someone to provide equivalent functionality entirely in userland if they really wanted it. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] UPDATE of partition key
On 24 February 2017 at 14:57, David G. Johnston wrote: > I dislike an error. I'd say that making partition "just work" here is > material for another patch. In this one an update of the partition key can > be documented as shorthand for delete-returning-insert with all the > limitations that go with that. If someone acceptably solves the ctid > following logic later it can be committed - I'm assuming there would be no > complaints to making things just work in a case where they only sorta > worked. Personally I don't think there's any hope that there will ever be cross-table ctids links. Maybe one day there will be a major new table storage format with very different capabilities than today but in the current architecture it seems like an impossible leap. I would expect everyone to come to terms with the basic idea that partition key updates are always going to be a corner case. The user defined the partition key and the docs should carefully explain to them the impact of that definition. As long as that explanation gives them something they can work with and manage the consequences of that's going to be fine. What I'm concerned about is that silently giving "wrong" answers in regular queries -- not even ones doing the partition key updates -- is something the user can't really manage. They have no way to rewrite the query to avoid the problem if some other user or part of their system is updating partition keys. They have no way to know the problem is even occurring. Just to spell it out -- it's not just "no-op updates" where the user sees 0 records updated. If I update all records where username='stark', perhaps to set the "user banned" flag and get back "9 records updated" and later find out that I missed a record because someone changed the department_id while my query was running -- how would I even know? How could I possibly rewrite my query to avoid that? The reason I suggested throwing a serialization failure was because I thought that would be the easiest short-cut to the problem. I had imagined having a bit pattern that indicated such a move would actually be a pretty minor change actually. I would actually consider using a normal update bitmask with InvalidBlockId in the ctid to indicate the tuple was updated and the target of the chain isn't available. That may be something we'll need in the future for other cases too. Throwing an error means the user has to retry their query but that's at least something they can do. Even if they don't do it automatically the ultimate user will probably just retry whatever operation errored out anyways. But at least their database isn't logically corrupted. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] SortSupport for macaddr type
On Sun, Feb 05, 2017 at 01:56:41PM -0800, Brandur Leach wrote: Hello Brandur, thanks for the updated patch! > > > * you used macaddr_cmp_internal() function name, for uuid > > the same function is named uuid_internal_cmp(). Using > > the same naming pattern is probably better. > > I was a little split on this one! It's true that UUID uses > `_internal_cmp`, but `_cmp_internal` is also used in a > number of places like `enum`, `timetz`, and `network`. I > don't have a strong feeling about it either way, so I've > changed it to `_internal_cmp` to match UUID as you > suggested. > Indeed, I should have checked more examples :/ There isn't any clear pattern for this, so I guess any one would be ok. > > * the function comment on macaddr_abbrev_convert() > > doesn't mention specific little-endian handling > > I tried to bake this into the comment text. Here are the > relevant lines of the amended version: > > * Packs the bytes of a 6-byte MAC address into a Datum and treats it as > an > * unsigned integer for purposes of comparison. On a 64-bit machine, > there > * will be two zeroed bytes of padding. The integer is converted to > native > * endianness to facilitate easy comparison. > > > * "There will be two bytes of zero padding on the least > > significant end" > > > > "least significant bits" would be better > > Also done. Here is the amended version: > > * On a 64-bit machine, zero out the 8-byte datum and copy the 6 bytes of > * the MAC address in. There will be two bytes of zero padding on the end > * of the least significant bits. > Thanks. I'm ok with this, but maybe a native english speaker would have a better opinion on this. > > * This patch will trigger quite a lot modifications after > > a pgindent run. Could you try to run pgindent on mac.c > > before sending an updated patch? > > Good call! I've run the new version through pgindent. > Thanks also, no more issue here. > Let me know if you have any further feedback and/or > suggestions. Thanks! One last thing, I noticed that you added: +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2); but the existing function is declared as static int32 macaddr_internal_cmp(macaddr *a1, macaddr *a2) I'd be in favor to declare both as int. After this, I think this patch will be ready for committer. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Reduce lock levels for reloptions
Patch to reduce lock levels of various relation-level options, following on from earlier work by Fabrizio, with additional work and documentation by me. Submitted to Sept CF, requested to start new thread to discuss https://www.postgresql.org/message-id/CA%2BTgmoYe5eDhjRodo3uOtVFGiDWwO2zGUp_mDHeSxuEqq-jS_A%40mail.gmail.com Earlier patch, dropped from 9.6 https://www.postgresql.org/message-id/CANP8%2Bj%2B0fuq2MWKvUsQTJFt8EF_z-Tyn-Q61J2A%2BVT2Uzuf-Rg%40mail.gmail.com -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services reduce_lock_levels_incl_comments.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] btree_gin and btree_gist for enums
On 02/25/2017 12:04 PM, Tom Lane wrote:
> I think it'd be better to leave DirectFunctionCallN alone and just invent
> a small number of CallerFInfoFunctionCallN support functions (maybe N=1
> and N=2 would be enough, at least for now).
>
>
See attached.
cheers
andrew
--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 3976496..66227e5 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -1276,6 +1276,54 @@ DirectFunctionCall9Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2,
return result;
}
+/*
+ * These functions work like the DirectFunctionCall functions except that
+ * they use the flinfo parameter to initialise the fcinfo for the call.
+ * The callee should not look at anything except the fn_mcxt and fn_extra.
+ * Anything else is likely to be bogus.
+ */
+
+Datum
+CallerFInfoFunctionCall1(PGFunction func, FmgrInfo *flinfo, Oid collation, Datum arg1)
+{
+ FunctionCallInfoData fcinfo;
+ Datum result;
+
+ InitFunctionCallInfoData(fcinfo, flinfo, 1, collation, NULL, NULL);
+
+ fcinfo.arg[0] = arg1;
+ fcinfo.argnull[0] = false;
+
+ result = (*func) (&fcinfo);
+
+ /* Check for null result, since caller is clearly not expecting one */
+ if (fcinfo.isnull)
+ elog(ERROR, "function %p returned NULL", (void *) func);
+
+ return result;
+}
+
+Datum
+CallerFInfoFunctionCall2(PGFunction func, FmgrInfo *flinfo, Oid collation, Datum arg1, Datum arg2)
+{
+ FunctionCallInfoData fcinfo;
+ Datum result;
+
+ InitFunctionCallInfoData(fcinfo, flinfo, 2, collation, NULL, NULL);
+
+ fcinfo.arg[0] = arg1;
+ fcinfo.arg[1] = arg2;
+ fcinfo.argnull[0] = false;
+ fcinfo.argnull[1] = false;
+
+ result = (*func) (&fcinfo);
+
+ /* Check for null result, since caller is clearly not expecting one */
+ if (fcinfo.isnull)
+ elog(ERROR, "function %p returned NULL", (void *) func);
+
+ return result;
+}
/*
* These are for invocation of a previously-looked-up function with a
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index a671480..739ce46 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -475,6 +475,19 @@ extern Datum DirectFunctionCall9Coll(PGFunction func, Oid collation,
Datum arg6, Datum arg7, Datum arg8,
Datum arg9);
+/*
+ * These functions work like the DirectFunctionCall functions except that
+ * they use the flinfo parameter to initialise the fcinfo for the call.
+ * The callee should not look at anything except the fn_mcxt and fn_extra.
+ * Anything else is likely to be bogus.
+ */
+
+extern Datum CallerFInfoFunctionCall1(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1);
+extern Datum CallerFInfoFunctionCall1(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1);
+
+
/* These are for invocation of a previously-looked-up function with a
* directly-computed parameter list. Note that neither arguments nor result
* are allowed to be NULL.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] btree_gin and btree_gist for enums
> The reason this is kind of scary is that it's just blithely assuming > that the function won't look at the *other* fields of the FmgrInfo. > If it did, it would likely get very confused, since those fields > would be describing the GIN support function, not the function we're > calling. I am sorry if it doesn't make sense, but wouldn't the whole thing be better, if we refactor enum.c to call only he internal functions with TypeCacheEntry just like the rangetypes? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] btree_gin and btree_gist for enums
Andrew Dunstan writes:
> On 02/24/2017 05:34 PM, Andrew Dunstan wrote:
>> It's occurred to me that we could reduce the code clutter in fmgr.c a
>> bit by turning the DirectFunctionCall{n]Coll functions into macros
>> calling these functions and passing NULL as the flinfo param.
> here's a patch along those lines. If there's agreement on this I can
> finish up the work on btree_gist and btree_gin supoport for enums.
I'm not really thrilled with doing it like that, for two reasons:
1. This makes it look like CallerFInfoFunctionCallN is the more basic,
common interface, which it isn't and never will be. At best, that's
confusing. And I don't like having only one documentation block
covering both interfaces; that does nothing for clarity either.
2. By my count there are over 500 uses of DirectFunctionCallN in the
backend. This will add an additional hidden parameter to each one,
implying code bloat and some fractional speed cost.
I think it'd be better to leave DirectFunctionCallN alone and just invent
a small number of CallerFInfoFunctionCallN support functions (maybe N=1
and N=2 would be enough, at least for now).
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] btree_gin and btree_gist for enums
On 02/24/2017 05:34 PM, Andrew Dunstan wrote:
>
> On 02/24/2017 02:55 PM, Andrew Dunstan wrote:
>> On 02/24/2017 11:02 AM, Tom Lane wrote:
I don't know what to call it either. In my test I used
CallerContextFunctionCall2 - not sure if that's quite right, but should
be close.
>>> CallerInfo? CallerFInfo? Or we could spell out CallerFmgrInfo but
>>> that seems a bit verbose.
>>>
>>>
>> I'll go with CallerFInfoFunctionCall2 etc.
>>
>> In the btree_gist system the calls to the routines like enum_cmp are
>> buried about three levels deep. I'm thinking I'll just pass the flinfo
>> down the stack and just call these routines at the bottom level.
>>
>>
>>
>
> It's occurred to me that we could reduce the code clutter in fmgr.c a
> bit by turning the DirectFunctionCall{n]Coll functions into macros
> calling these functions and passing NULL as the flinfo param.
>
here's a patch along those lines. If there's agreement on this I can
finish up the work on btree_gist and btree_gin supoport for enums.
cheers
andrew
--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 3976496..d565d3e 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -1006,19 +1006,30 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
*-
*/
-/*
- * These are for invocation of a specifically named function with a
+
+/* These are for invocation of a specifically named function with a
* directly-computed parameter list. Note that neither arguments nor result
- * are allowed to be NULL. Also, the function cannot be one that needs to
- * look at FmgrInfo, since there won't be any.
+ * are allowed to be NULL.
+ *
+ * If the flinfo parameter is not NULL then this used to supply context
+ * to the called function. The callee should only use the passed in fn_mcxt
+ * and fn_extra. Anything else is likely to be bogus.
+ *
+ * But the fn_extra can be used for example for caching results
+ * (see enum_cmp for an example).
+ *
+ * No context is supplied if the parameter is NULL. See DirectFunctionCall
+ * macros. In this case the function must not try to look at FmgrInfo, since
+ * there won't be one and it is likely to result in an access violation.
*/
Datum
-DirectFunctionCall1Coll(PGFunction func, Oid collation, Datum arg1)
+CallerFInfoFunctionCall1(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1)
{
FunctionCallInfoData fcinfo;
Datum result;
- InitFunctionCallInfoData(fcinfo, NULL, 1, collation, NULL, NULL);
+ InitFunctionCallInfoData(fcinfo, flinfo, 1, collation, NULL, NULL);
fcinfo.arg[0] = arg1;
fcinfo.argnull[0] = false;
@@ -1033,12 +1044,13 @@ DirectFunctionCall1Coll(PGFunction func, Oid collation, Datum arg1)
}
Datum
-DirectFunctionCall2Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2)
+CallerFInfoFunctionCall2(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1, Datum arg2)
{
FunctionCallInfoData fcinfo;
Datum result;
- InitFunctionCallInfoData(fcinfo, NULL, 2, collation, NULL, NULL);
+ InitFunctionCallInfoData(fcinfo, flinfo, 2, collation, NULL, NULL);
fcinfo.arg[0] = arg1;
fcinfo.arg[1] = arg2;
@@ -1055,13 +1067,14 @@ DirectFunctionCall2Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2)
}
Datum
-DirectFunctionCall3Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2,
- Datum arg3)
+CallerFInfoFunctionCall3(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1, Datum arg2,
+ Datum arg3)
{
FunctionCallInfoData fcinfo;
Datum result;
- InitFunctionCallInfoData(fcinfo, NULL, 3, collation, NULL, NULL);
+ InitFunctionCallInfoData(fcinfo, flinfo, 3, collation, NULL, NULL);
fcinfo.arg[0] = arg1;
fcinfo.arg[1] = arg2;
@@ -1080,13 +1093,14 @@ DirectFunctionCall3Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2,
}
Datum
-DirectFunctionCall4Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2,
- Datum arg3, Datum arg4)
+CallerFInfoFunctionCall4(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1, Datum arg2,
+ Datum arg3, Datum arg4)
{
FunctionCallInfoData fcinfo;
Datum result;
- InitFunctionCallInfoData(fcinfo, NULL, 4, collation, NULL, NULL);
+ InitFunctionCallInfoData(fcinfo, flinfo, 4, collation, NULL, NULL);
fcinfo.arg[0] = arg1;
fcinfo.arg[1] = arg2;
@@ -1107,13 +1121,14 @@ DirectFunctionCall4Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2,
}
Datum
-DirectFunctionCall5Coll(PGFunction func, Oid collation, Datum arg1, Datum arg2,
- Datum arg3, Datum arg4, Datum arg5)
+CallerFInfoFunctionCall5(PGFunction func, FmgrInfo *flinfo,
+ Oid collation, Datum arg1, Datum arg2,
+ Datum arg3, Datum arg4, Datum arg5)
{
FunctionCallI
Re: [HACKERS] Enabling parallelism for queries coming from SQL or other PL functions
On Sat, Feb 25, 2017 at 5:12 PM, Amit Kapila wrote: > Sure, but that should only happen if the function is *not* declared as > parallel safe (aka in parallel safe functions, we should not generate > parallel plans). So basically we want to put a restriction that parallel-safe function can not use the parallel query? This will work but it seems too restrictive to me. Because by marking function parallel safe we enable it to be used with the outer parallel query that is fine. But, that should not restrict the function from using the parallel query if it's used with the other outer query which is not having the parallel plan(or function is executed directly). > >> So I think we >> may need some check during execution time as well? >> > > Right, I also think we need some mechanism where if the user has not > marked the parallel safe functions appropriately, then such executions > should result in error. For example, if parallel-safe function calls > a parallel-unsafe function which contains either write statement or > statement that could generate a parallel plan, then we should not > allow execution of such queries. We already have safeguard checks at > most places like write statements (see heap_update), however, I think > we need a similar check in ExecGather. How about we allow parallel-safe functions to create a parallel plan but whenever it's used from an unsafe place i.e. already in the parallel mode we don't allow to launch worker? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Automatic cleanup of oldest WAL segments with pg_receivexlog
On Feb 25, 2017 15:00, "Michael Paquier" wrote: On Sat, Feb 25, 2017 at 10:32 PM, Magnus Hagander wrote: > Oh, I definitely think such a command should be able to take a placeholder > like %f telling which segment it has just processed. In fact, I'd consider > it one of the most important features of it :) I cannot think about any other meaningful variables, do you? Not offhand. But one thing that could go to the question of parameter name - what if we finish something that's not a segment. During a time line switch for example, we also get other files don't we? We probably want to trigger at least some command in that case - either with an argument or by a different parameter? /Magnus
Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)
On Sat, Feb 25, 2017 at 10:50:57AM +0530, Pavan Deolasee wrote: > > On Fri, Feb 24, 2017 at 9:47 PM, Robert Haas wrote: > And there are no bugs, right? :-) > > Yeah yeah absolutely nothing. Just like any other feature committed to > Postgres > so far ;-) > > I need to polish the chain conversion patch a bit and also add missing support > for redo, hash indexes etc. Support for hash indexes will need overloading of > ip_posid bits in the index tuple (since there are no free bits left in hash > tuples). I plan to work on that next and submit a fully functional patch, > hopefully before the commit-fest starts. > > (I have mentioned the idea of overloading ip_posid bits a few times now and > haven't heard any objection so far. Well, that could either mean that nobody > has read those emails seriously or there is general acceptance to that idea.. > I > am assuming latter :-)) Yes, I think it is the latter. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Automatic cleanup of oldest WAL segments with pg_receivexlog
On Sat, Feb 25, 2017 at 10:32 PM, Magnus Hagander wrote: > Oh, I definitely think such a command should be able to take a placeholder > like %f telling which segment it has just processed. In fact, I'd consider > it one of the most important features of it :) I cannot think about any other meaningful variables, do you? -- 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] Proposal for changes to recovery.conf API
On Fri, Feb 24, 2017 at 7:39 PM, Simon Riggs wrote:
> New patch version implementing everything you requested, incl docs and
> tap tests.
The TAP changes look good to me. I thought that more diffs would have
been necessary.
> The patch as offered here is what I've been asked to do by everybody
> as well as I can do it. I'm very happy to receive comments and to
> rework the design based upon further feedback.
This meritates a summary after all has happened on this thread and
this topic. After reading this patch, here is what this is doing:
- The existing standby_mode in recovery.conf is removed, replaced by
standby.trigger to decide if a node in recovery should be put in
standby mode.
- recovery.trigger can be used to put a node in recovery. When both
standby.trigger and recovery.trigger are specified, standby.trigger
takes priority.
- Two GUC parameters, recovery_target_type and recovery_target_value,
replace all the existing recovery target parameters.
- pg_basebackup -R generates recovery.conf.auto.
- trigger_file is removed.
FWIW, my only complain is about the removal of trigger_file, this is
useful to detect a trigger file on a different partition that PGDATA!
Keeping it costs also nothing..
> I'm not completely convinced this is a great design, so I'm happy to
> hear input. pg_basebackup -R is the main wrinkle.
Yeah, I can imagine that. It is easy to append a new file to a
tarball, harder to add data to an existing file perhaps?
> The timeline handling has a bug at present that I'm working on, but
> I'm not worried it constitutes a major problem. Obviously it will be
> fixed before commit, but the patch needs more discussion
> now/yesterday.
Running the tests I can see failures in 004_timeline_switch.pl, which
is what you likely mention here, as well as a failure in
008_fsm_truncation.pl. I can also see that 009_twophase.pl is
freezing.
> All parameters are set at PGC_POSTMASTER for now.
Thanks for considering that, this makes the review of this patch
easier, and that's complicated enough as-is.
-test_recovery_standby('XID + time + name + LSN',
- 'standby_9', $node_master, \@recovery_params, "5000", $lsn5);
+# Tests for multiple targets no longer needed from 10.0 onwards
I would be fine without any comments as well. What's the point of
mentioning a past state here?
# Switch standby 2 to replay from standby 1
-rmtree($node_standby_2->data_dir . '/recovery.conf');
+#still needed?rmtree($node_standby_2->data_dir . '/recovery.conf');
my $connstr_1 = $node_standby_1->connstr;
No need to worry here, this can be removed.
osx:func.sgml:18159:19:X: reference to non-existent ID "RECOVERY-TARGET-NAME"
osx:release-9.1.sgml:9814:23:X: reference to non-existent ID
"RECOVERY-TARGET-NAME"
osx:recovery-config.sgml:311:26:X: reference to non-existent ID
"RECOVERY-TARGET-XID"
osx:recovery-config.sgml:310:43:X: reference to non-existent ID
"RECOVERY-TARGET-TIME"
osx:release-9.4.sgml:8034:27:X: reference to non-existent ID "RECOVERY-TARGET"
Documentation fails to compile.
+The database server can also be started "in recovery" a term that covers
+using the server as a standby or for executing a targeted
recovery. Typically
+standby mode would be used to provide high availability and/or read
+scalability, whereas a targeted recovery is used to recover from data loss.
Double quotes directly used in a documentation paragraph are not nice.
You should add a comma after "in recovery".
+To start the server in standby mode create a zero-length file
+called standby.signalstandby.signal
Zero-length file is not mandatory. Only creating a file is.
+
+ Parameter settings may be changed in
postgresql.conf or
+ by executing the ALTER SYSTEM. Changes will take some
+ time to take effect, so changes made while not in paused state may not
+ have the desired effect in all cases.
+
But those parameters are PGC_POSTMASTER?!
+ By default, recovery will recover to the end of the WAL log. An earlier
+ stopping point may be specified using recovery_target_type
+ and in most cases also recovery_target_value, plus
the optional
+ parameters recovery_target_inclusive,
+ recovery_targeti_timeline and
recovery_target_action.
+
"recovery will recover" is a bad phrasing, I would change that to
"recovery will process".
There is also a typo => s/targeti/target/.
+
+ Parameter settings may be changed only at server start, though later
+ patches may add this capability.
+
I would just say that "new values for those parameters are considered
only at restart of the server". There is no need to speculate about a
potential future in the documentation. If nothing happens this would
remain incorrect.
By default, a standby server restores WAL records from the
-primary as soon as possible. It may be useful to have a time-delayed
+sending server as soon as possible. It may be useful to have
a time-delayed
Perha
Re: [HACKERS] Automatic cleanup of oldest WAL segments with pg_receivexlog
On Fri, Feb 24, 2017 at 3:52 AM, Michael Paquier wrote: > On Fri, Feb 24, 2017 at 1:10 AM, Magnus Hagander > wrote: > > I'm not sure this logic belongs in pg_receivexlog. If we put the decision > > making there, then we lock ourselves into one "type of policy". > > > > Wouldn't this one, along with some other scenarios, be better provided by > > the "run command at end of segment" function that we've talked about > before? > > And then that external command could implement whatever aging logic > would be > > appropriate for the environment? > > OK, I forgot a bit about this past discussion. So let's say that we > have a command, why not also allow users to use at will a marker %f to > indicate the file name just completed? One use case here is to scan > the file for the oldest and/or newest timestamps of the segment just > finished to do some retention policy with something else in charge of > the cleanup. > > The option name would be --end-segment-command? Any better ideas of names? > Oh, I definitely think such a command should be able to take a placeholder like %f telling which segment it has just processed. In fact, I'd consider it one of the most important features of it :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Automatic cleanup of oldest WAL segments with pg_receivexlog
On Fri, Feb 24, 2017 at 3:56 AM, Jim Nasby wrote: > On 2/23/17 8:47 PM, Michael Paquier wrote: > >> Anything else than measured in bytes either requires a lookup at the >> file timestamp, which is not reliable with noatime or a lookup at WAL >> itself to decide when is the commit timestamp that matches the oldest >> point in time of the backup policy. >> > > An indication that it'd be nice to have a better way to store this > information as part of a base backup, or the archived WAL files. > > That could be made performance >> wise with an archive command. With pg_receivexlog you could make use >> of the end-segment command to scan the completely written segment for >> this data before moving on to the next one. At least it gives an >> argument for having such a command. David Steele mentioned that he >> could make use of such a thing. >> > > BTW, I'm not opposed to an end-segment command; I'm just saying I don't > think having it would really help users very much. > > It might not help end users directly, but it could certainly help tools-developers. At least that's what I'd think. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] logical replication access control patches
On 2/18/17 18:06, Stephen Frost wrote: > I'm not convinced that it really makes sense to have PUBLICATION of a > table be independent from the rights an owner of a table has. We don't > allow other ALTER commands on objects based on GRANT'able rights, in > general, so I'm not really sure that it makes sense to do so here. The REFERENCES and TRIGGER privileges are very similar in principle. > The downside of adding these privileges is that we're burning through > the last few bits in the ACLMASK for a privilege that doesn't really > seem like it's something that would be GRANT'd in general usage. I don't see any reason why we couldn't increase the size of AclMode if it becomes necessary. > I'm certainly all for removing the need for users to be the superuser > for such commands, just not sure that they should be GRANT'able > privileges instead of privileges which the owner of the relation or > database has. Then you couldn't set up a replication structure involving tables owned by different users without resorting to blunt instruments like having everything owned by the same user or using superusers. -- 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] Proposal : Parallel Merge Join
On Sat, Feb 25, 2017 at 3:22 PM, Dilip Kumar wrote: > On Sat, Feb 25, 2017 at 11:29 AM, Amit Kapila wrote: >> It seems you have forgotten to change in the below check: >> >> + if (innerpath != NULL && >> + innerpath->parallel_safe && >> + (cheapest_startup_inner == NULL || >> + cheapest_startup_inner->parallel_safe == false || >> + compare_path_costs(innerpath, cheapest_startup_inner, >> + STARTUP_COST) < 0)) > > Oops, Fixed. > Thanks, patch looks good to me (apart from some of the code comments which might need some adjustment and I think it is better if Committer takes care of same as required rather than we argue about it) and I have changed the status as Ready For Committer. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enabling parallelism for queries coming from SQL or other PL functions
On Fri, Feb 24, 2017 at 11:30 AM, Dilip Kumar wrote:
> On Fri, Feb 24, 2017 at 10:06 AM, Amit Kapila wrote:
>> We have a below check in standard_planner() (!IsParallelWorker())
>> which should prohibit generating parallel plan inside worker, if that
>> is what you are seeing, then we might need a similar check at other
>> places.
>>
>> if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
>> IsUnderPostmaster &&
>> dynamic_shared_memory_type != DSM_IMPL_NONE &&
>> parse->commandType == CMD_SELECT &&
>> !parse->hasModifyingCTE &&
>> max_parallel_workers_per_gather > 0 &&
>> !IsParallelWorker() &&
>> !IsolationIsSerializable())
>> {
>> /* all the cheap tests pass, so scan the query tree */
>> glob->maxParallelHazard = max_parallel_hazard(parse);
>> glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
>> }
>
> Ok, I see. But, the problem with PL functions is that plan might have
> already generated in previous execution of the function and during
> that time outer query might not be running in parallel.
>
Sure, but that should only happen if the function is *not* declared as
parallel safe (aka in parallel safe functions, we should not generate
parallel plans).
> So I think we
> may need some check during execution time as well?
>
Right, I also think we need some mechanism where if the user has not
marked the parallel safe functions appropriately, then such executions
should result in error. For example, if parallel-safe function calls
a parallel-unsafe function which contains either write statement or
statement that could generate a parallel plan, then we should not
allow execution of such queries. We already have safeguard checks at
most places like write statements (see heap_update), however, I think
we need a similar check in ExecGather.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow pg_dumpall to work without pg_authid
On 22 February 2017 at 07:33, Robins Tharakan wrote: > Stephen, > > On 20 February 2017 at 08:50, Stephen Frost wrote: >> >> The other changes to use pg_roles instead of pg_authid when rolpassword >> isn't being used look like they should just be changed to use pg_roles >> instead of using one or the other. That should be an independent patch >> from the one which adds the option we are discussing. > > > Sure. Attached are 2 patches, of which 1 patch just replaces pg_authid with > pg_roles in pg_dumpall. The only exceptions there are buildShSecLabels() > & pg_catalog.binary_upgrade_set_next_pg_authid_oid() which I thought > should still use pg_authid. Patch, and life, is simpler if we use just one or the other, IMHO. >> Perhaps --no-role-passwords instead? >> > Makes Sense. The updated patch uses this name. > >> >> > pg_dumpall --no-pgauthid --globals-only > a.sql >> >> Does that then work with a non-superuser account on a regular PG >> instance also? If not, I'd like to suggest that we consider follow-on >> patches to provide options for whatever else currently requires >> superuser on a regular install. >> > If I understand that correctly, the answer is Yes. I didn't test all db > objects, > but trying to do a pg_dumpall using a non-priviledge user does successfully > complete with all existing users dumped successfully. > > pg_dumpall --globals-only --no-role-password > a.sql It looks to me like you'd need to use --no-security-labels as well, in most cases since that accesses pg_authid also. The patch seemed to be missing substantial chunks of coding, so I've added those also. I've also added checks to prevent it running with other mutually exclusive options. Reworded doc changes also. Tested, but no tests added for this. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services pg_dumpall_no_role_passwords.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] Proposal : Parallel Merge Join
On Sat, Feb 25, 2017 at 11:29 AM, Amit Kapila wrote: > It seems you have forgotten to change in the below check: > > + if (innerpath != NULL && > + innerpath->parallel_safe && > + (cheapest_startup_inner == NULL || > + cheapest_startup_inner->parallel_safe == false || > + compare_path_costs(innerpath, cheapest_startup_inner, > + STARTUP_COST) < 0)) Oops, Fixed. > > > + /* Found a cheap (or even-cheaper) sorted parallel safer path */ > > typo > /safer/safe Fixed > > Note - Change the patch status in CF app (to Needs Review) whenever > you post a new patch. I could see that your other patch also needs a > similar update in CF app. Will changes, Thanks. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com parallel_mergejoin_v7.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] Logical replication existing data copy
On 2017-02-25 00:40, Petr Jelinek wrote: 0001-Use-asynchronous-connect-API-in-libpqwalreceiver.patch 0002-Fix-after-trigger-execution-in-logical-replication.patch 0003-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION.patch snapbuild-v3-0001-Reserve-global-xmin-for-create-slot-snasphot-export.patch snapbuild-v3-0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch snapbuild-v3-0003-Fix-xl_running_xacts-usage-in-snapshot-builder.patch snapbuild-v3-0004-Skip-unnecessary-snapshot-builds.patch 0001-Logical-replication-support-for-initial-data-copy-v6.patch Here are some results. There is improvement although it's not an unqualified success. Several repeat-runs of pgbench_derail2.sh, with different parameters for number-of-client yielded an output file each. Those show that logrep is now pretty stable when there is only 1 client (pgbench -c 1). But it starts making mistakes with 4, 8, 16 clients. I'll just show a grep of the output files; I think it is self-explicatory: Output-files (lines counted with grep | sort | uniq -c): -- out_20170225_0129.txt 250 -- pgbench -c 1 -j 8 -T 10 -P 5 -n 250 -- All is well. -- out_20170225_0654.txt 25 -- pgbench -c 4 -j 8 -T 10 -P 5 -n 24 -- All is well. 1 -- Not good, but breaking out of wait (waited more than 60s) -- out_20170225_0711.txt 25 -- pgbench -c 8 -j 8 -T 10 -P 5 -n 23 -- All is well. 2 -- Not good, but breaking out of wait (waited more than 60s) -- out_20170225_0803.txt 25 -- pgbench -c 16 -j 8 -T 10 -P 5 -n 11 -- All is well. 14 -- Not good, but breaking out of wait (waited more than 60s) So, that says: 1 clients: 250x success, zero fail (250 not a typo, ran this overnight) 4 clients: 24x success, 1 fail 8 clients: 23x success, 2 fail 16 clients: 11x success, 14 fail I want to repeat what I said a few emails back: problems seem to disappear when a short wait state is introduced (directly after the 'alter subscription sub1 enable' line) to give the logrep machinery time to 'settle'. It makes one think of a timing error somewhere (now don't ask me where..). To show that, here is pgbench_derail2.sh output that waited 10 seconds (INIT_WAIT in the script) as such a 'settle' period works faultless (with 16 clients): -- out_20170225_0852.txt 25 -- pgbench -c 16 -j 8 -T 10 -P 5 -n 25 -- All is well. QED. (By the way, no hanged sessions so far, so that's good) thanks Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
