Re: [HACKERS] upgrade failure from 9.5 to head
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: * Andres Freund (and...@anarazel.de) wrote: Hm. That issue doesn't particularly concern me. Having all .so's available in the installation seems like a pretty basic requirement. Security labels are by far not the only things that'll fail without an extension's .so present, no? It's certainly an issue that postgis users are familiar with. Really? What aspect of postgis requires mucking with shared_preload_libraries? Having to have the libraries in place is what I was getting at, which is what Andres was also talking about, if I understood correctly. Even without having to muck with shared_preload_libraries though, you had better have those libraries in place if you want things to work. Having to also deal with shared_preload_libraries for some cases doesn't strike me as a huge issue. If you ask me, shared_preload_libraries was only ever meant as a performance optimization. If user-visible DDL behavior depends on a library being preloaded that way, that feature is broken. There are some cases where we probably don't care enough to provide a proper solution, but I'm not sure why we would think that security labels fall in the don't-really-give-a-damn-if-it-works class. I agree that labels are important and that we really want the label provider loaded from shared_preload_libraries. I also understand that shared_preload_libraries was originally intended as a performance optimization and that the security labels system has ended up changing that. I don't believe that'll be the last thing which we want to be loaded and running from the very start (if we end up with auditing as an extension, or any hooks in the postmaster that we provide for extensions to use, etc). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] upgrade failure from 9.5 to head
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: Really? What aspect of postgis requires mucking with shared_preload_libraries? Having to have the libraries in place is what I was getting at, which is what Andres was also talking about, if I understood correctly. Right, I agree with that: you need to have installed all the software you're using, where installed means the executable files are built and placed where they need to be in the filesystem. Having to also deal with shared_preload_libraries for some cases doesn't strike me as a huge issue. I think it is, especially if what we're offering as a workaround is write a custom script and make sure that your pg_upgrade wrapper script has an option to call that halfway through. Rube Goldberg would be proud. It's possible that the problem here is not so much reliance on shared_preload_libraries as it is that there's no provision in pg_upgrade for dealing with the need to set it. But one way or the other, this is a usability fail. 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] upgrade failure from 9.5 to head
Tom Lane wrote: Andres Freund and...@anarazel.de writes: Ick! So the dummy_seclabel test more or less only works by accident if I see that correctly. The .so is only loaded because the CREATE EXTENSION in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG C. I set it up that way on purpose, because there doesn't seem to be any other reasonable way. (It wasn't like that in the original contrib module). But on reflection, doesn't this mean that the entire implementation of SECURITY LABEL is broken? Heck, see the *very first* hunk of this patch: https://www.postgresql.org/message-id/CACACo5R4VwGddt00qtPmhUhtd%3Dokwu2ECM-j20B6%2BcmgtZF6mQ%40mail.gmail.com This was found to be necessary during deparsing of SECURITY LABEL command, and only of that command -- all other DDL is able to pass back the OID of the corresponding object, so that the deparsing code can look up the object in catalogs. But for security label providers there is no catalog, and there is no way to obtain the identify of the label provider that I can see. Thus the ugly hack in the patch: the parse node has to be altered during execution to make sure the provider is correctly set up for deparse to be able to obtain it. That to me seemed pretty broken, but I found no better way. I don't think this is dummy_seclabel's fault. -- Álvaro Herrerahttp://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] dblink: add polymorphic functions.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2015 08:56 AM, Corey Huinker wrote: On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane t...@sss.pgh.pa.us Not sure why inserting a variable name is so much better than inserting a type name? In a polymorphic function, I don't know the return type. It's whatever type was specified on the function call. Say I've written a function with a function like outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) returns setof anyelement Remind me where you get p_rowtype at runtime again? At some point you are still having to calculate it, no? - -- Joe Conway -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuPv8AAoJEDfy90M199hlYCwP/i0/berqnjXFch6zFM5TDZ6h +UxQxMmVg933U6Cdoc+huMz8Hiotix76BZVgHOa8xI7Vktx1y5D7o7auczg31v4o BkzbJFX9ziK5TXZm5wEvtTPNJOOq25AqvHzmrwr4JnPvyAOCKQASTqhIi95mdflZ tGI+fuI4Dlkj76JaIuZjIh1rKMdycHsRmVfULVx6luR5LwzhX/iyW66pMkNHPYui 7K3DP2hF4HR6xoq4Jvj+HX1MSLLRAjm6+emx6YKnkSkxCQvL5EzwupWWhr7khrYk QV0fwbAG5JtQJlid/DxezUFgpi2qs2AoPy2ub7JyEZjY0ULt4ehOx9/pzk0ATMNo e3jB1H9BHTCiy5tY3VZBCe0uQ3lqiNalyUPcwYviS3yciuNg78rIkCQKe2KritZw t0BY0SMASjbgIINlOLkDCg3HaYi3JujdbwSR5dG41RxaMej3MMieh3Opyg9bfZhI TxWLsec7FPW/T23wPGk1aQZ7IbLRlOz95fJlAua6g1d5m6GWE3lyRQAQP+QFNWfX /dmAy0Hvyp3a1wRsFrsG1W+GoyNV1pwfjXp+QlDDhGf8G/Q4l5s7OzZRcLs0O67Z 3K7Jn2k8ps4ZxA5yqRZdl3aAuaOpf3iqffQEeWQqXx7UAM5Sd8qorOJXpNhw+JtX 9W1YQ3eNgGkDfcOa9JLm =P2ff -END PGP SIGNATURE- -- 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] LWLock deadlock and gdb advice
On 2015-07-29 09:23:32 -0700, Jeff Janes wrote: On Tue, Jul 28, 2015 at 9:06 AM, Jeff Janes jeff.ja...@gmail.com wrote: I've reproduced it again against commit b2ed8edeecd715c8a23ae462. It took 5 hours on a 8 core Intel(R) Xeon(R) CPU E5-2650. I also reproduced it in 3 hours on the same machine with both JJ_torn_page and JJ_xid set to zero (i.e. turned off, no induced crashes), so the fault-injection patch should not be necessary to get the issue.. Hm, that's a single socket or dual socket E5-2650 system? That CPU itself has 8 cores, and can work in a dual socket system, that's why I ask. -- 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] more RLS oversights
On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. Yeah, I was debating that. Although I do think the name transformWhereClause() is unfortunate. Maybe it ought to be renamed transformBooleanExpr() or something similar? There are a couple of places in test_rls_hooks.c that also need updating. Right -- thanks I think that check_agglevels_and_constraints() and transformWindowFuncCall() could be made to emit more targeted error messages for EXPR_KIND_POLICY, for example aggregate functions are not allowed in policy USING and WITH CHECK expressions. ok Thanks for the review. Joe -- 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] Reduce ProcArrayLock contention
On Wed, Jul 29, 2015 at 10:54 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Jul 27, 2015 at 8:47 PM, Robert Haas robertmh...@gmail.com wrote: On Sat, Jul 25, 2015 at 12:42 AM, Amit Kapila amit.kapil...@gmail.com wrote: I thought that internal API will automatically take care of it, example for msvc it uses _InterlockedCompareExchange64 which if doesn't work on 32-bit systems or is not defined, then we have to use 32-bit version, but I am not certain about that fact. Instead of using pg_atomic_uint64, how about using pg_atomic_uint32 and storing the pgprocno rather than the pointer directly? Good Suggestion! I think this can work the way you are suggesting and I am working on same. Here I have one question, do you prefer to see the code for this optimisation be done via some LWLock interface as Pavan is suggesting? I am not very sure if LWLock is a good interface for this work, but surely I can encapsulate it into different functions rather than doing everything in ProcArrayEndTransaction. I would try to avoid changing lwlock.c. It's pretty easy when so doing to create mechanisms that work now but make further upgrades to the general lwlock mechanism difficult. I'd like to avoid that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] upgrade failure from 9.5 to head
Stephen Frost sfr...@snowman.net writes: More generally, I completely agree that this is something which we can improve upon. It doesn't seem like a release blocker or something which we need to fix in the back branches though. No, it's not a release blocker; it's been like this since we invented security labels, which seems to have been 9.1. But security labels were toys in 9.1, and this deficiency is one reason they still are toys. We need to have a to-do item to get rid of it, rather than claiming things are just fine as they are. 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] LWLock deadlock and gdb advice
On Tue, Jul 28, 2015 at 9:06 AM, Jeff Janes jeff.ja...@gmail.com wrote: On Tue, Jul 28, 2015 at 7:06 AM, Andres Freund and...@anarazel.de wrote: Hi, On 2015-07-19 11:49:14 -0700, Jeff Janes wrote: After applying this patch to commit fdf28853ae6a397497b79f, it has survived testing long enough to convince that this fixes the problem. What was the actual workload breaking with the bug? I ran a small variety and I couldn't reproduce it yet. I'm not saying there's no bug, I just would like to be able to test my version of the fixes... It was the torn-page fault-injection code here: https://drive.google.com/open?id=0Bzqrh1SO9FcEfkxFb05uQnJ2cWg0MEpmOXlhbFdyNEItNmpuek1zU2gySGF3Vk1oYXNNLUE It is not a minimal set, I don't know if all parts of this are necessary to rerproduce it. The whole crash-recovery cycling might not even be important. I've reproduced it again against commit b2ed8edeecd715c8a23ae462. It took 5 hours on a 8 core Intel(R) Xeon(R) CPU E5-2650. I also reproduced it in 3 hours on the same machine with both JJ_torn_page and JJ_xid set to zero (i.e. turned off, no induced crashes), so the fault-injection patch should not be necessary to get the issue.. Cheers, Jeff
Re: [HACKERS] more RLS oversights
On 07/29/2015 08:46 AM, Joe Conway wrote: On 07/29/2015 01:01 AM, Dean Rasheed wrote: The CreatePolicy() and AlterPolicy() changes look OK to me, but the RemovePolicyById() change looks to be unnecessary --- RemovePolicyById() is called only from doDeletion(), which in turned is called only from deleteOneObject(), which already invokes the drop hooks. So ISTM that RemovePolicyById() doesn't need to do anything, right? Seems correct. Will remove that change and commit it that way. Pushed to HEAD and 9.5. Joe -- 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] LWLock deadlock and gdb advice
On Wed, Jul 29, 2015 at 9:26 AM, Andres Freund and...@anarazel.de wrote: On 2015-07-29 09:23:32 -0700, Jeff Janes wrote: On Tue, Jul 28, 2015 at 9:06 AM, Jeff Janes jeff.ja...@gmail.com wrote: I've reproduced it again against commit b2ed8edeecd715c8a23ae462. It took 5 hours on a 8 core Intel(R) Xeon(R) CPU E5-2650. I also reproduced it in 3 hours on the same machine with both JJ_torn_page and JJ_xid set to zero (i.e. turned off, no induced crashes), so the fault-injection patch should not be necessary to get the issue.. Hm, that's a single socket or dual socket E5-2650 system? That CPU itself has 8 cores, and can work in a dual socket system, that's why I ask. It is a virtual machine, and I think a property of the VM software is that any given virtual machine can only run on one socket of the underlying hardware. The real machine is dual socket, though. Cheers, Jeff
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 16:52, Joe Conway joe.con...@crunchydata.com wrote: On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. Yeah, I was debating that. Although I do think the name transformWhereClause() is unfortunate. Maybe it ought to be renamed transformBooleanExpr() or something similar? I expect it's probably being used by other people's code though, so renaming it might cause pain for quite a few people. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dblink: add polymorphic functions.
On Wed, Jul 29, 2015 at 12:14 PM, Joe Conway m...@joeconway.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2015 08:56 AM, Corey Huinker wrote: On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane t...@sss.pgh.pa.us Not sure why inserting a variable name is so much better than inserting a type name? In a polymorphic function, I don't know the return type. It's whatever type was specified on the function call. Say I've written a function with a function like outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) returns setof anyelement Remind me where you get p_rowtype at runtime again? At some point you are still having to calculate it, no? Say I've got a table my_partitioned_table (key1 integer, key2 integer, metric1 integer, metric2 integer); And I've got many partitions on that table. My code lets you do something like this: select key1, key2, sum(metric1) as a_sum_of_sums, sum(metric2) as another_sum_of_sums from execute_buncha_queries(null::my_partitioned_table, 'connection_string_thats_just_a_loopback', array['select key1, key2, sum(metric1), sum(metric2) from my_partition_p1 group by 1,2', 'select key1, key2, sum(metric1), sum(metric2) from my_partition_p2 group by 1,2', ...]) group by 1,2 All those queries happen to return a shape the same as my_partitioned_table. The query takes the partially summed values, spread out across a lot of processors, and does the lighter work of summing the sums. The function execute_buncha_queries fires off those string queries async, enough to fill up X number of processors, fetches results as they happen, and keeps feeding the processors queries until it runs out. But execute_buncha_queries needs to send back results in the shape of whatever value was passed in the first parameter. I can't put a cast around execute_buncha_queries because the function won't know how to cast the results coming back from dblink. select * from execute_lotta_queries(null::my_table_or_type,'connection_string_to_remote_db', array['query 1','query 2','query 3']) Now, it's up to the user to make sure that all the query strings return a query of shape my_table_or_type, but that's a runtime problem. And obviously, there are a lot of connection strings, but that's too much detail for the problem at hand.
Re: [HACKERS] more RLS oversights
On 07/29/2015 01:01 AM, Dean Rasheed wrote: The CreatePolicy() and AlterPolicy() changes look OK to me, but the RemovePolicyById() change looks to be unnecessary --- RemovePolicyById() is called only from doDeletion(), which in turned is called only from deleteOneObject(), which already invokes the drop hooks. So ISTM that RemovePolicyById() doesn't need to do anything, right? Seems correct. Will remove that change and commit it that way. Joe -- 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] upgrade failure from 9.5 to head
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: Having to also deal with shared_preload_libraries for some cases doesn't strike me as a huge issue. I think it is, especially if what we're offering as a workaround is write a custom script and make sure that your pg_upgrade wrapper script has an option to call that halfway through. Rube Goldberg would be proud. It's possible that the problem here is not so much reliance on shared_preload_libraries as it is that there's no provision in pg_upgrade for dealing with the need to set it. But one way or the other, this is a usability fail. Why would it be pg_upgrade? When using it, you initdb the new cluster yourself and you can configure the postgresql.conf in there however you like before running the actual pg_upgrade. Sure, if you're using a wrapper then you need to deal with that, but if you want pg_upgrade to handle configuration options in postgresql.conf then you're going to have to pass config info to the wrapper script anyway, which would then pass it to pg_upgrade. The wrapper script may even already deal with this if it copies the old postgresql.conf into place (which I think the Debian one might do, with a bit of processing for anything that needs to be addressed between the major versions.. not looking at it right now though). More generally, I completely agree that this is something which we can improve upon. It doesn't seem like a release blocker or something which we need to fix in the back branches though. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] dblink: add polymorphic functions.
On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Corey Huinker corey.huin...@gmail.com writes:
On Wed, Jul 29, 2015 at 3:48 AM, Heikki Linnakangas hlinn...@iki.fi
wrote:
Let's pursue the CAST(srf() AS row_rtype) syntax that Joe suggested
upthread (
http://www.postgresql.org/message-id/559a9643.9070...@joeconway.com).
For
some reason, the discussion went on around the details of the submitted
patch after that, even though everyone seemed to prefer the CAST()
syntax.
I'm all for adding that syntax, but it wouldn't be useful for my purposes
unless row_rtype could be a variable, and my understanding is that it
can't.
Not sure why inserting a variable name is so much better than inserting a
type name?
regards, tom lane
Apologies in advance if I'm going over things you already know. Just trying
to package up the problem statement into something easily digestible.
In a polymorphic function, I don't know the return type. It's whatever type
was specified on the function call.
Say I've written a function with a function like
outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) returns
setof anyelement
And inside that function is a series (probably a handful, but potentially
thousands) of async dblink calls, and their corresponding calls to
dblink_get_result(), which currently return setof record, each of which
needs to be casted to whatever anyelement happens to be given this
execution.
Currently, I have to look up p_rowtype in pg_attribute and pg_class, render
the column specs as valid SQL, and compose the query as a string
fetch_values_query := 'select * from dblink_get_result($1) as t ( ' ||
'c1 type, c2 othertype, ... ' || ')';
and then execute that dynamically like so:
return query execute fetch_values_query using l_connection_name;
It would be nice if I didn't have to resort to dynamic SQL do to this.
If the CAST() function is implemented, but does not allow to cast as a
variable, then I'm in the same boat:
fetch_values_query := 'select * from CAST(dblink_get_result($1) as ' ||
pg_typeof(p_rowtype) || ')';
Admittedly, that's a bit cleaner, but I'm still executing that dynamic SQL
once per connection I made, and there could be a lot of them.
If there were dblink() functions that returned polymorphic results, it
would be a non issue:
dblink_send_query('conn1','select * from
thing_i_know_is_shaped_like_my_rowtype')
...
return query select * from dblink_get_result_any(p_rowtype,'conn1');
I'm all for the expanded capabilities of CAST(), but I have a specific need
for polymorphic dblink() functions.
Re: [HACKERS] dblink: add polymorphic functions.
Corey Huinker corey.huin...@gmail.com writes: On Wed, Jul 29, 2015 at 10:48 AM, Tom Lane t...@sss.pgh.pa.us wrote: Not sure why inserting a variable name is so much better than inserting a type name? In a polymorphic function, I don't know the return type. It's whatever type was specified on the function call. Say I've written a function with a function like outer_polymorphic_function(p_rowtype anyelement,p1 ,p2,p3, ...) returns setof anyelement And inside that function is a series (probably a handful, but potentially thousands) of async dblink calls, and their corresponding calls to dblink_get_result(), which currently return setof record, each of which needs to be casted to whatever anyelement happens to be given this execution. Yeah. I would argue that the appropriate fix for that involves allowing the p_rowtype%TYPE syntax to be used in the CAST. I think right now you can only use %TYPE in DECLARE sections, but that's a limitation that causes problems in more situations than just this one. Mind you, making that work might be less than trivial :-( ... but it would have uses well beyond dblink(). 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] Supporting TAP tests with MSVC and Windows
On 07/29/2015 10:13 AM, Michael Paquier wrote: On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier michael.paqu...@gmail.com wrote: An updated patch is attached. Attached is v9, that fixes conflicts with 01f6bb4 and recent commits that added TAP tests in pg_basebackup series. Thanks, committed with some more tweaking. Peter just added a slurp_file() function to TestLib.pm, so I used that to replace the call to 'cat' instead of my previous snippet. I also fixed the issue in the pg_basebackup test, that LSN is not necessarily 8 characters long, slightly differently. Calling pg_xlogfile_name seemed a bit indirect. I expanded the documentation on how to download and install IPC::Run. I instructed to add PERL5LIB to buildenv.pl, pointing it to the extracted IPC-Run-0.94/lib directory. Your phrasing of putting it into PERL5LIB was pretty vague, and in fact the PERL5LIB environment variable was overwritten in vcregress.pl, so just setting the PERL5LIB variable wouldn't have worked. I changed vcregress.pl to not do that. I considered moving the instructions on downloading and installing IPC::Run in the Requirements section, instead of the Running the Regression Tests section. But there are additional instructions on downloading and installing more dependencies in the Building the Documentation section too. The other dependencies in the Requirements section affect the built binaries. Well, except for the Diff utility. We're not totally consistent, but I think it's OK as it is. Also I added an equivalent of --enable-tap-tests for this MSVC stuff, removed afterwards by Heikki. Do we want it or not? On Unix, that option controls whether make check-world runs the TAP tests, but there is no equivalent of check-world on Windows. Maybe there should be, but as long as there isn't, the only thing that the option did was to stop vcregress tapcheck from working, if didn't enable it in the config file first. That seems silly, so I removed it. We'll need it if we add an equivalent of make check-world to vcregress, but let's cross that bridge when we get there. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LWLock deadlock and gdb advice
On 07/29/2015 04:10 PM, Andres Freund wrote: On 2015-07-29 14:22:23 +0200, Andres Freund wrote: On 2015-07-29 15:14:23 +0300, Heikki Linnakangas wrote: Ah, ok, that should work, as long as you also re-check the variable's value after queueing. Want to write the patch, or should I? I'll try. Shouldn't be too hard. What do you think about something roughly like the attached? Hmm. Imagine this: Backend A has called LWLockWaitForVar(X) on a lock, and is now waiting on it. The lock holder releases the lock, and wakes up A. But before A wakes up and sees that the lock is free, another backend acquires the lock again. It runs LWLockAcquireWithVar to the point just before setting the variable's value. Now A wakes up, sees that the lock is still (or again) held, and that the variable's value still matches the old one, and goes back to sleep. The new lock holder won't wake it up until it updates the value again, or to releases the lock. I think that's OK given the current usage of this in WALInsertLocks, but it's scary. At the very least you need comments to explain that race condition. You didn't like the new LW_FLAG_VAR_SET flag and the API changes I proposed? That eliminates the race condition. Either way, there is a race condition that if the new lock holder sets the variable to the *same* value as before, the old waiter won't necessarily wake up even though the lock was free or had a different value in between. But that's easier to explain and understand than the fact that the value set by LWLockAcquireWithVar() might not be seen by a waiter, until you release the lock or update it again. Another idea is to have LWLockAcquire check the wait queue after setting the variable, and wake up anyone who's already queued up. You could just call LWLockUpdateVar() from LWLockAcquireCommon to set the variable. I would prefer the approach from my previous patch more, though. That would avoid having to acquire the spinlock in LWLockAcquire altogether, and I actually like the API change from that independently from any correctness issues. The changes in LWLockWaitForVar() and LWLockConflictsWithVar() seem OK in principle. You'll want to change LWLockConflictsWithVar() so that the spinlock is held only over the value = *valptr line, though; the other stuff just modifies local variables and don't need to be protected by the spinlock. Also, when you enter LWLockWaitForVar(), you're checking if the lock is held twice in a row; first at the top of the function, and again inside LWLockConflictsWithVar. You could just remove the quick test at the top. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Division by zero in planner.c:grouping_planner()
On Wed, Jul 29, 2015 at 11:26 AM, Piotr Stefaniak postg...@piotr-stefaniak.me wrote: + Assert(path_rows != 0); if (tuple_fraction = 1.0) tuple_fraction /= path_rows; } This does not sounds right: path_rows only used when tuple_fractions = 1.0. So the new Assert is an overkill. Regards, Qingqing -- 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] more RLS oversights
On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. Ok -- I went back to using transformWhereClause. I'd still prefer to change the name -- more than half the uses of the function are for other than EXPR_KIND_WHERE -- but I don't feel that strongly about it. There are a couple of places in test_rls_hooks.c that also need updating. done I think that check_agglevels_and_constraints() and transformWindowFuncCall() could be made to emit more targeted error messages for EXPR_KIND_POLICY, for example aggregate functions are not allowed in policy USING and WITH CHECK expressions. done Unless there are other comments/objections, will commit this later today. Joe diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index d8b4390..bcf4a8f 100644 *** a/src/backend/commands/policy.c --- b/src/backend/commands/policy.c *** CreatePolicy(CreatePolicyStmt *stmt) *** 534,545 qual = transformWhereClause(qual_pstate, copyObject(stmt-qual), ! EXPR_KIND_WHERE, POLICY); with_check_qual = transformWhereClause(with_check_pstate, copyObject(stmt-with_check), ! EXPR_KIND_WHERE, POLICY); /* Fix up collation information */ --- 534,545 qual = transformWhereClause(qual_pstate, copyObject(stmt-qual), ! EXPR_KIND_POLICY, POLICY); with_check_qual = transformWhereClause(with_check_pstate, copyObject(stmt-with_check), ! EXPR_KIND_POLICY, POLICY); /* Fix up collation information */ *** AlterPolicy(AlterPolicyStmt *stmt) *** 707,713 addRTEtoQuery(qual_pstate, rte, false, true, true); qual = transformWhereClause(qual_pstate, copyObject(stmt-qual), ! EXPR_KIND_WHERE, POLICY); /* Fix up collation information */ --- 707,713 addRTEtoQuery(qual_pstate, rte, false, true, true); qual = transformWhereClause(qual_pstate, copyObject(stmt-qual), ! EXPR_KIND_POLICY, POLICY); /* Fix up collation information */ *** AlterPolicy(AlterPolicyStmt *stmt) *** 730,736 with_check_qual = transformWhereClause(with_check_pstate, copyObject(stmt-with_check), ! EXPR_KIND_WHERE, POLICY); /* Fix up collation information */ --- 730,736 with_check_qual = transformWhereClause(with_check_pstate, copyObject(stmt-with_check), ! EXPR_KIND_POLICY, POLICY); /* Fix up collation information */ diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 478d8ca..e33adf5 100644 *** a/src/backend/parser/parse_agg.c --- b/src/backend/parser/parse_agg.c *** check_agglevels_and_constraints(ParseSta *** 373,378 --- 373,385 case EXPR_KIND_WHERE: errkind = true; break; + case EXPR_KIND_POLICY: + if (isAgg) + err = _(aggregate functions are not allowed in POLICY USING and WITH CHECK expressions); + else + err = _(grouping operations are not allowed in POLICY USING and WITH CHECK expressions); + + break; case EXPR_KIND_HAVING: /* okay */ break; *** transformWindowFuncCall(ParseState *psta *** 770,775 --- 777,785 case EXPR_KIND_WHERE: errkind = true; break; + case EXPR_KIND_POLICY: + err = _(aggregate functions are not allowed in POLICY USING and WITH CHECK expressions); + break; case EXPR_KIND_HAVING: errkind = true; break; diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 0ff46dd..fa77ef1 100644 *** a/src/backend/parser/parse_expr.c --- b/src/backend/parser/parse_expr.c *** transformSubLink(ParseState *pstate, Sub *** 1672,1677 --- 1672,1678 case EXPR_KIND_FROM_SUBSELECT: case EXPR_KIND_FROM_FUNCTION: case EXPR_KIND_WHERE: + case EXPR_KIND_POLICY: case EXPR_KIND_HAVING: case EXPR_KIND_FILTER: case EXPR_KIND_WINDOW_PARTITION: *** ParseExprKindName(ParseExprKind exprKind *** 3173,3178 --- 3174,3181 return function in FROM; case EXPR_KIND_WHERE: return WHERE; + case EXPR_KIND_POLICY: + return POLICY; case EXPR_KIND_HAVING: return HAVING; case EXPR_KIND_FILTER: diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h index 7ecaffc..5249945 100644 *** a/src/include/parser/parse_node.h --- b/src/include/parser/parse_node.h *** typedef enum ParseExprKind *** 63,69 EXPR_KIND_INDEX_PREDICATE, /* index predicate */ EXPR_KIND_ALTER_COL_TRANSFORM, /* transform expr in
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 20:36, Joe Conway joe.con...@crunchydata.com wrote: On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. Ok -- I went back to using transformWhereClause. I'd still prefer to change the name -- more than half the uses of the function are for other than EXPR_KIND_WHERE -- but I don't feel that strongly about it. There are a couple of places in test_rls_hooks.c that also need updating. done I think that check_agglevels_and_constraints() and transformWindowFuncCall() could be made to emit more targeted error messages for EXPR_KIND_POLICY, for example aggregate functions are not allowed in policy USING and WITH CHECK expressions. done Unless there are other comments/objections, will commit this later today. In the final error s/aggregate/window/. Other than that it looks good to me. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
Joe Conway wrote: On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. Ok -- I went back to using transformWhereClause. I'd still prefer to change the name -- more than half the uses of the function are for other than EXPR_KIND_WHERE -- but I don't feel that strongly about it. Currently the comment about it says for WHERE and allied, maybe this should be a bit more explicit. I think it was originally for things like WHERE and HAVING, and usage slowly extended beyond that. Does it make sense to consider the USING clause in CREATE/ALTER POLICY as an allied clause of WHERE? I don't particularly think so. I'm just talking about a small rewording of the comment. I think that check_agglevels_and_constraints() and transformWindowFuncCall() could be made to emit more targeted error messages for EXPR_KIND_POLICY, for example aggregate functions are not allowed in policy USING and WITH CHECK expressions. done + case EXPR_KIND_POLICY: + if (isAgg) + err = _(aggregate functions are not allowed in POLICY USING and WITH CHECK expressions); + else + err = _(grouping operations are not allowed in POLICY USING and WITH CHECK expressions); I think this reads a bit funny. What's a POLICY USING clause? I expect that translators will treat the two words POLICY USING as a single token, and the result is not going to make any sense. Maybe in a policy's USING and WITH CHECK expressions, or perhaps in policies's USING and WITH CHECK exprs, not sure. -- Álvaro Herrerahttp://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] Remaining 'needs review' patchs in July commitfest
Robert Haas robertmh...@gmail.com writes: On Tue, Jul 28, 2015 at 3:51 PM, Heikki Linnakangas hlinn...@iki.fi wrote: plpgsql raise statement with context Impasse. Everyone wants this feature in some form, but no consensus on whether to do this client-side or server-side. +1 for server-side. Does anyone other than you even think that the client side is a reasonable way to go? Yes. This is presupposing on the server side what the client will want to display. dblink: add polymorphic result functions Seems pretty ugly to me to add a dummy argument to functions, just so that you can specify the result type. The problem it's trying to solve is real, though. Should we take it as it is, or wait for some cleaner approach? +1 for take it. If we wait for something better, we may be waiting a long time. -1. We have a clear design spec for a solution that fixes this for much more than just dblink. I don't feel a need to paste on an ugly, single-use band-aid in such a hurry. If we get close to 9.6 and there is no better fix, we could reconsider; but now is not the time to give up on pursuing the better solution. Asynchronous execution on postgres-fdw If we're going to have some sort of general-purpose Parallel node support soon, should we just forget about this? Or is it still useful? This adds a fair amount of new infrastructure, for a fairly niche feature.. IMHO, that's an extremely important feature. I believe that it will not be obsoleted by other parallelism work. Again, this seems like rushing through a single-use band-aid while work is still active on more general solutions. I think we should put this off and see whether it makes sense *after* the general parallel query stuff is (more nearly) functional. Even if it still makes sense then, it may well need a rewrite. Actually, I think that some of the infrastructure that it introduces may be quite helpful for parallelism as well, but I haven't looked at in detail yet. Then steal that stuff as and when you need 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] dblink: add polymorphic functions.
On Wed, Jul 29, 2015 at 12:53 PM, Joe Conway m...@joeconway.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2015 09:40 AM, Corey Huinker wrote: Say I've got a table my_partitioned_table (key1 integer, key2 integer, metric1 integer, metric2 integer); And I've got many partitions on that table. My code lets you do something like this: select key1, key2, sum(metric1) as a_sum_of_sums, sum(metric2) as another_sum_of_sums from execute_buncha_queries(null::my_partitioned_table, 'connection_string_thats_just_a_loopback', array['select key1, key2, sum(metric1), sum(metric2) from my_partition_p1 group by 1,2', 'select key1, key2, sum(metric1), sum(metric2) from my_partition_p2 group by 1,2', ...]) group by 1,2 I can't put a cast around execute_buncha_queries because the function won't know how to cast the results coming back from dblink. Ok, gotcha. So Tom's nearby comment about allowing the p_rowtype%TYPE syntax to be used in the CAST is spot on (as usual). In other words, to get a complete solution for you we would need to make both things work, so you could do this inside plpgsql: select * from cast(dblink(connstr, sql) as p_rowtype%TYPE); Would this be a pl/pgsql only solution? merlin -- 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] more RLS oversights
On 07/29/2015 02:56 PM, Joe Conway wrote: On 07/29/2015 02:04 PM, Alvaro Herrera wrote: Why not just in policy expressions? There's no third kind that does allow these. WFM Sold! Will do it that way. Committed/pushed to HEAD and 9.5. Joe -- 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] upgrade failure from 9.5 to head
On 07/29/2015 11:28 AM, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: Really? What aspect of postgis requires mucking with shared_preload_libraries? Having to have the libraries in place is what I was getting at, which is what Andres was also talking about, if I understood correctly. Right, I agree with that: you need to have installed all the software you're using, where installed means the executable files are built and placed where they need to be in the filesystem. Having to also deal with shared_preload_libraries for some cases doesn't strike me as a huge issue. I think it is, especially if what we're offering as a workaround is write a custom script and make sure that your pg_upgrade wrapper script has an option to call that halfway through. Rube Goldberg would be proud. It's possible that the problem here is not so much reliance on shared_preload_libraries as it is that there's no provision in pg_upgrade for dealing with the need to set it. But one way or the other, this is a usability fail. FWIW, having the test driver add the shared_preload_libraries setting got over this hump - the shared library is indeed present in my setup. The next hump is this, in restoring contrib_regression_test_ddl_parse: pg_restore: creating FUNCTION public.text_w_default_in(cstring) pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534 FUNCTION text_w_default_in(cstring) buildfarm pg_restore: [archiver (db)] could not execute query: ERROR: pg_type OID value not set when in binary upgrade mode Command was: CREATE FUNCTION text_w_default_in(cstring) RETURNS text_w_default LANGUAGE internal STABLE STRICT AS $$texti... Is this worth bothering about, or should I simply remove the database before trying to upgrade? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Improving test coverage of extensions with pg_dump
I have reviewed this patch and it compiles runs and the new test case passes. The code is also clean and the test seems like a useful regression test. What I do not like though is how the path src/test/tables_fk/t/ tells us nothing about what features are of PostgreSQL are tested here. For this I personally prefer the earlier versions where I think that was clear. Another though: would it be worthwhile to also add an assertion to check if the data really was restored properly or would that just be redundant code? -- Andreas -- 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] more RLS oversights
Robert Haas wrote: On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway joe.con...@crunchydata.com wrote: The equivalent message for functions is: .. are not allowed in functions in FROM So how does this sound: ... are not allowed in policies in USING and WITH CHECK expressions or perhaps more simply: ... are not allowed in policies in USING and WITH CHECK Awkward. The in policies in phrasing is just hard to read. Yeah. Besides, it's not really the same thing. Why not just in policy expressions? There's no third kind that does allow these. WFM -- Álvaro Herrerahttp://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] Typo in a comment in set_foreignscan_references
On Tue, Jul 28, 2015 at 3:16 AM, Amit Langote langote_amit...@lab.ntt.co.jp wrote: Attached fixes a minor typo: s/custom/foreign/g Committed, thanks. -- 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] more RLS oversights
On 07/29/2015 01:26 PM, Robert Haas wrote: On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think this reads a bit funny. What's a POLICY USING clause? I expect that translators will treat the two words POLICY USING as a single token, and the result is not going to make any sense. Maybe in a policy's USING and WITH CHECK expressions, or perhaps in policies's USING and WITH CHECK exprs, not sure. Yeah, I don't see why we would capitalize POLICY there. The equivalent message for functions is: .. are not allowed in functions in FROM So how does this sound: ... are not allowed in policies in USING and WITH CHECK expressions or perhaps more simply: ... are not allowed in policies in USING and WITH CHECK Joe -- 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] more RLS oversights
On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway joe.con...@crunchydata.com wrote: On 07/29/2015 01:26 PM, Robert Haas wrote: On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think this reads a bit funny. What's a POLICY USING clause? I expect that translators will treat the two words POLICY USING as a single token, and the result is not going to make any sense. Maybe in a policy's USING and WITH CHECK expressions, or perhaps in policies's USING and WITH CHECK exprs, not sure. Yeah, I don't see why we would capitalize POLICY there. The equivalent message for functions is: .. are not allowed in functions in FROM So how does this sound: ... are not allowed in policies in USING and WITH CHECK expressions or perhaps more simply: ... are not allowed in policies in USING and WITH CHECK Awkward. The in policies in phrasing is just hard to read. Why not just in policy expressions? There's no third kind that does allow these. -- 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] dblink: add polymorphic functions.
Merlin Moncure mmonc...@gmail.com writes: On Wed, Jul 29, 2015 at 12:53 PM, Joe Conway m...@joeconway.com wrote: Ok, gotcha. So Tom's nearby comment about allowing the p_rowtype%TYPE syntax to be used in the CAST is spot on (as usual). In other words, to get a complete solution for you we would need to make both things work, so you could do this inside plpgsql: select * from cast(dblink(connstr, sql) as p_rowtype%TYPE); Would this be a pl/pgsql only solution? Well, it would depend on how we fixed %TYPE, but my thought is that we should teach the core parser to accept variable%TYPE anywhere that a suitable variable is in scope. The core already allows related syntaxes in some utility commands, but not within DML commands. (I am not sure offhand if the existing core syntax is exactly compatible with what plpgsql does, though; and that could be a problem.) 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] more RLS oversights
On 07/29/2015 02:04 PM, Alvaro Herrera wrote: Why not just in policy expressions? There's no third kind that does allow these. WFM Sold! Will do it that way. Joe -- 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] Remaining 'needs review' patchs in July commitfest
On Tue, Jul 28, 2015 at 3:51 PM, Heikki Linnakangas hlinn...@iki.fi wrote: plpgsql raise statement with context Impasse. Everyone wants this feature in some form, but no consensus on whether to do this client-side or server-side. +1 for server-side. Does anyone other than you even think that the client side is a reasonable way to go? dblink: add polymorphic result functions Seems pretty ugly to me to add a dummy argument to functions, just so that you can specify the result type. The problem it's trying to solve is real, though. Should we take it as it is, or wait for some cleaner approach? +1 for take it. If we wait for something better, we may be waiting a long time. Asynchronous execution on postgres-fdw If we're going to have some sort of general-purpose Parallel node support soon, should we just forget about this? Or is it still useful? This adds a fair amount of new infrastructure, for a fairly niche feature.. IMHO, that's an extremely important feature. I believe that it will not be obsoleted by other parallelism work. Actually, I think that some of the infrastructure that it introduces may be quite helpful for parallelism as well, but I haven't looked at in detail yet. The core design concern that I have is what to do about this: Append - Scan - Scan - Scan Right now, we start each scan when the previous scan finishes. But what if - either through parallelism or through this patch - we could launch scans 2, 3, etc. before scan 1 completes? If there's a Limit node above this somewhere we may regret our decision bitterly. But if there isn't, we may do an enormous amount of good by starting that plan early on the speculation that we will in fact need the results eventually. I haven't applied a whole lot of brainpower to this problem yet; I'm sort of hoping someone else will have a good idea. Kyotaro Horiguchi's approach seems to be to say that we don't need to solve this problem at this stage and that we should just not worry about the possible waste of resources on the remote machine for now; fix it later, as part of a future patch. I'm not terribly confident in that approach, but I don't currently have anything better to propose. -- 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] Planner debug views
On Tue, Jul 28, 2015 at 6:13 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Another point is that we decided a long time ago that EXPLAIN's plain-text
output format is not intended to be machine-parsable, and so objecting to
a design on the grounds that it makes machine parsing harder is pretty
wrongheaded. I'd think there is plenty of room for dropping in additional
output data in the non-text output formats.
I think this will work, for example, I can put several sections of the
JSON output:
{
plan: {
// original EXPLAIN plan tree sits here
},
paths:{
// paths considered sits here
}
// ...
}
But still, it requires an extra step for user: he will needs to
programming to read through output (easier) and persists into a table for
later query.
Can we simplify above with foreign table methods? There are two major
concerns about this method per previous discussions: security and
usability. I think the main cause is the sharing foreign table design. How
about we put foreign table in separate pg_stat_tmp/pid folders, similar
to what Alvaro proposes, and similar to /proc file system. Meanwhile, we
introduce a function to help user create foreign table mapping to these
files. This looks solves the security and usability issues to me:
postgres=# select pg_debug_planner_init();
Foreign table 'pg_planner_rels', 'pg_planner_paths' created.
postgres=# EXPLAIN (debug_planner=on, ...) ...
...
postgres=# select * from pg_planner_paths;
...
Thoughts?
Qngqing
--
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: multiple psql option -c
Hi
here is proof concept patch
It should be cleaned, but it demonstrates a work well
[pavel@localhost psql]$ ./psql -C 'select 10 x; select 20 y;' -C \l
postgres
x
10
(1 row)
y
20
(1 row)
List of databases
Name| Owner | Encoding | Collate |Ctype| Access
privileges
---+--+--+-+-+---
postgres | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
template0 | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=c/postgres +
| | | | |
postgres=CTc/postgres
template1 | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=c/postgres +
| | | | |
postgres=CTc/postgres
(3 rows)
2015-07-28 18:46 GMT+02:00 Andrew Dunstan and...@dunslane.net:
On 07/28/2015 11:52 AM, Pavel Stehule wrote:
2015-07-28 15:16 GMT+02:00 Andrew Dunstan and...@dunslane.net mailto:
and...@dunslane.net:
On 07/28/2015 12:08 AM, Pavel Stehule wrote:
2015-07-28 5:24 GMT+02:00 Pavel Stehule
pavel.steh...@gmail.com mailto:pavel.steh...@gmail.com
mailto:pavel.steh...@gmail.com
mailto:pavel.steh...@gmail.com:
2015-07-27 21:57 GMT+02:00 Andrew Dunstan
and...@dunslane.net mailto:and...@dunslane.net
mailto:and...@dunslane.net mailto:and...@dunslane.net:
On 07/27/2015 02:53 PM, Pavel Stehule wrote:
I am trying to run parallel execution
psql -At -c select datname from pg_database
postgres |
xargs -n 1 -P 3 psql -c select current_database()
I don't think it's going to be a hugely important
feature, but
I don't see a problem with creating a new option (-C seems
fine) which would have the same effect as if the arguments
were contatenated into a file which is then used with
-f. IIRC
-c has some special characteristics which means it's
probably
best not to try to extend it for this feature.
ok, I'll try to write patch.
I have a question. Can be -C option multiple?
The SQL is without problem, but what about \x command?
postgres=# \dt \dn select 10;
No relations found.
List of schemas
┌──┬───┐
│ Name │ Owner │
╞══╪═══╡
└──┴───┘
(0 rows)
\dn: extra argument 10; ignored
I don't understand the question.
You should include one sql or psql command per -C option, ISTM. e.g.
psql -C '\dt' -C '\dn' -C 'select 10;'
Isn't that what we're talking about with this whole proposal?
I am searching some agreement, how to solve a current -c limits. I am
100% for psql -C '\dt' -C '\dn' -C 'select 10;'
I think you're probably best off leaving -c alone. If there are issues to
be solved for -c they should be handled separately from the feature we
agree on.
cheers
andrew
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
new file mode 100644
index b6cef94..bfd5bf5
*** a/src/bin/psql/mainloop.c
--- b/src/bin/psql/mainloop.c
*** MainLoop(FILE *source)
*** 43,48
--- 43,49
volatile promptStatus_t prompt_status = PROMPT_READY;
volatile int count_eof = 0;
volatile bool die_on_error = false;
+ Commands *cmd = pset.commands;
/* Save the prior command source */
FILE *prev_cmd_source;
*** MainLoop(FILE *source)
*** 135,140
--- 136,156
prompt_status = PROMPT_READY;
line = gets_interactive(get_prompt(prompt_status));
}
+ else if (pset.commands != NULL)
+ {
+ pset.cur_cmd_interactive = false;
+
+ if (cmd != NULL)
+ {
+ line = cmd-actions;
+ cmd = cmd-next;
+ }
+ else
+ {
+ successResult = EXIT_SUCCESS;
+ break;
+ }
+ }
else
{
line = gets_fromFile(source);
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
new file mode 100644
index d34dc28..46d2a81
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*** enum trivalue
*** 77,82
--- 77,88
TRI_YES
};
+ typedef struct _Commands
+ {
+ char *actions;
+ struct _Commands *next;
+ } Commands;
+
typedef struct _psqlSettings
{
PGconn *db;/* connection to backend */
*** typedef struct _psqlSettings
*** 129,134
--- 135,141
const char *prompt2;
const char *prompt3;
PGVerbosity verbosity; /* current error verbosity level */
+ Commands *commands;
} PsqlSettings;
extern PsqlSettings pset;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
new file mode 100644
index 28ba75a..cbceb91
*** a/src/bin/psql/startup.c
---
Re: [HACKERS] CustomScan and readfuncs.c
On Sun, Jul 26, 2015 at 11:14 AM, Tom Lane t...@sss.pgh.pa.us wrote: Um ... wait a second. There is no support in readfuncs for any plan node type, and never has been, and I seriously doubt that there ever should be. As KaiGai says, the parallel query stuff contemplates changing this; how else will we get the plan from the master to each worker? We could invent a bespoke serialization format, but that seems to have exactly nothing to recommend it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 7/29/15 8:00 AM, Andres Freund wrote: As far as I understand this subthread the goal is to have a pg_basebackup that internally creates a slot, so it can guarantee that all the required WAL is present till streamed out by -X stream/fetch. The problem with just creating a slot is that it'd leak if pg_basebackup is killed, or the connection breaks. The idea with the ephemeral slot is that it'd automatically kept alive until the base backup is complete, but would also automatically be dropped if the base backup fails. I don't think anyone actually said that. I think the goal was to add the functionality that pg_basebackup can optionally create a (persistent) replication slot, both for its own use and for later use by streaming. Just so you don't have to call pg_create...slot() or pg_receivexlog separately to create the slot. And it was pointed out that this created slot would need to be removed if pg_basebackup failed for some reason. What you describe also seems useful, but is a different sub-issue. -- 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] more RLS oversights
On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think this reads a bit funny. What's a POLICY USING clause? I expect that translators will treat the two words POLICY USING as a single token, and the result is not going to make any sense. Maybe in a policy's USING and WITH CHECK expressions, or perhaps in policies's USING and WITH CHECK exprs, not sure. Yeah, I don't see why we would capitalize POLICY there. -- 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] dblink: add polymorphic functions.
Ok, gotcha. So Tom's nearby comment about allowing the p_rowtype%TYPE syntax to be used in the CAST is spot on (as usual). In other words, to get a complete solution for you we would need to make both things work, so you could do this inside plpgsql: select * from cast(dblink(connstr, sql) as p_rowtype%TYPE); where potentially connstr, sql, p_rowtype are all passed to plpgsql as arguments. Correct? Correct.
Re: [HACKERS] Planner debug views
Qingqing Zhou wrote: Can we simplify above with foreign table methods? There are two major concerns about this method per previous discussions: security and usability. I think the main cause is the sharing foreign table design. I think foreign data wrappers are great. I do not think that we should try to shape every problem to look like foreign data so that we can solve it with a foreign data wrapper. I am a bit nervous that this keeps being brought up. -- Álvaro Herrerahttp://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] dblink: add polymorphic functions.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2015 09:40 AM, Corey Huinker wrote: Say I've got a table my_partitioned_table (key1 integer, key2 integer, metric1 integer, metric2 integer); And I've got many partitions on that table. My code lets you do something like this: select key1, key2, sum(metric1) as a_sum_of_sums, sum(metric2) as another_sum_of_sums from execute_buncha_queries(null::my_partitioned_table, 'connection_string_thats_just_a_loopback', array['select key1, key2, sum(metric1), sum(metric2) from my_partition_p1 group by 1,2', 'select key1, key2, sum(metric1), sum(metric2) from my_partition_p2 group by 1,2', ...]) group by 1,2 I can't put a cast around execute_buncha_queries because the function won't know how to cast the results coming back from dblink. Ok, gotcha. So Tom's nearby comment about allowing the p_rowtype%TYPE syntax to be used in the CAST is spot on (as usual). In other words, to get a complete solution for you we would need to make both things work, so you could do this inside plpgsql: select * from cast(dblink(connstr, sql) as p_rowtype%TYPE); where potentially connstr, sql, p_rowtype are all passed to plpgsql as arguments. Correct? Joe -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuRMWAAoJEDfy90M199hlT5sP/3GuKbvZC7Ods3i2SqtTGbTo raFiKM87ZznswldNjDHVBEp+OntXzb0JbPUf6n/YJoEJGWE95wb40jez5snAV9lO aJ7CD9P235OgVh7QVTeWIW5Who8Yj1Xx6NF/Gz/06pGoAXQj1QoznnUPYixur4dT znjWW3XY6eFpfLzIBKIJmcskOKezgqj2F/kRJpgGYVaEm3okVkjubDjmPM5Vbaaa sd/lDI5ByceIImzL8LaFeBWwUGLYRsP02zamfPiz3p1zMb+ViBvS+NiBG0kMZMCt bzy6g0kxbLaxkKy/oEQXqinCNY3hEn8eZ7w4Os/3Zk9PCacZAKDrXfqBDTuE6zio wy/nwBnoTvdBSk0gl+MKoVlmoy0iAV7Hl/R/KtdNdpCTL4Ja6R5V2c/sjWazxAg4 PymaTXi4/SNWTFwAYGJUfGL+i3CMNQfp4U/tGTVPGFZ8thss7FTVNIVR6ZcAzuPM EHYDZ8cGaewqDF/HdPlJl4ypxF3aS8tzzApiFVpu35+2WHEacwljDV40l8z9Ee1V E7R0oxG55fgRJKvLSn5Oj59U2iBXgcu0zLLhBhaVyOYhcIZbWe6xvF1UN/RNcOuz Se10lYSXCTmz3q61HyCNnWFcOtgYSeFA3Lc79vgxJoZWmwnpHYoFEjQxr3qHFeeK svkoix7k7t7YZUXGebbg =vM1F -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transactions involving multiple postgres foreign servers
On Wed, Jul 29, 2015 at 6:58 AM, Ashutosh Bapat ashutosh.ba...@enterprisedb.com wrote: A user may set atomic_foreign_transaction to ON to guarantee atomicity, IOW it throws error when atomicity can not be guaranteed. Thus if application accidentally does something to a foreign server, which doesn't support 2PC, the transaction would abort. A user may set it to OFF (consciously and takes the responsibility of the result) so as not to use 2PC (probably to reduce the overheads) even if the foreign server is 2PC compliant. So, I thought a GUC would be necessary. We can incorporate the behaviour you are suggesting by having atomic_foreign_transaction accept three values full (ON behaviour), partial (behaviour you are describing), none (OFF behaviour). Default value of this GUC would be partial. Will that be fine? I don't really see the point. If the user attempts a distributed transaction involving FDWs that can't support atomic foreign transactions, then I think it's reasonable to assume that they want that to work rather than arbitrarily fail. The only situation in which it's desirable for that to fail is when the user doesn't realize that the FDW in question doesn't support atomic foreign commit, and the error message warns them that their assumptions are unfounded. But can't the user find that out easily enough by reading the documentation? So I think that in practice the full value of this GUC would get almost zero use; I think that nearly everyone will be happy with what you are here calling partial or none. I'll defer to any other consensus that emerges, but that's my view. I think that we should not change the default behavior. Currently, the only behavior is not to attempt 2PC. Let's stick with that. About table level atomic commit attribute, I agree that some foreign tables might hold more critical data than others from the same server, but I am not sure whether only that attribute should dictate the atomicity or not. A transaction collectively might need to be atomic even if the individual tables it modified are not set atomic_commit attribute. So, we need a transaction level attribute for atomicity, which may be overridden by a table level attribute. Should we add support to the table level atomicity setting as version 2+? I'm not hung up on the table-level attribute, but I think having a server-level attribute rather than a global GUC is a good idea. However, I welcome other thoughts on that. We should consider other possible designs as well; the choices we make here may have a significant impact on usability. I looked at other RBDMSes like IBM's federated database or Oracle. They support only full behaviour as described above with some optimizations like LRO. But, I would like to hear about other options. Yes, I hope others will weigh in. HandleForeignTransaction is not very descriptive, and I think you're jamming together things that ought to be separated. Let's have a PrepareForeignTransaction and a ResolvePreparedForeignTransaction. Done, there are three hooks now 1. For preparing a foreign transaction 2. For resolving a prepared foreign transaction 3. For committing/aborting a running foreign transaction (more explanation later) (2) and (3) seem like the same thing. I don't see any further explanation later in your email; what am I missing? IP might be fine, but consider altering dbname option or dropping it; we won't find the prepared foreign transaction in new database. Probably not, but I think that's the DBA's problem, not ours. I think we should at least warn the user that there exist a prepared foreign transaction on given foreign server or user mapping; better even if we let FDW decide which options are allowed to be altered when there exists a foreign prepared transaction. The later requires some surgery in the way we handle the options. We can consider that, but I don't think it's an essential part of the patch, and I'd punt it for now in the interest of keeping this as simple as possible. Is this a change from the current behavior? There is no current behaviour defined per say. My point is that you had some language in the email describing what happens if the GUC is turned off. You shouldn't have to describe that, because there should be absolutely zero difference. If there isn't, that's a problem for this patch, and probably a subject for a different one. I have added a built-in pg_fdw_remove() (or any suitable name), which removes the prepared foreign transaction entry from the memory and disk. The function needs to be called before attempting PITR. If the recovery points to a past time without removing file, we abort the recovery. In such case, a DBA can remove the foreign prepared transaction file manually before recovery. I have added a hint with that effect in the error message. Is that enough? That seems totally broken. Before PITR, the database might be inconsistent, in which case you can't call
Re: [HACKERS] upgrade failure from 9.5 to head
On Wed, Jul 29, 2015 at 11:28 AM, Tom Lane t...@sss.pgh.pa.us wrote: It's possible that the problem here is not so much reliance on shared_preload_libraries as it is that there's no provision in pg_upgrade for dealing with the need to set it. But one way or the other, this is a usability fail. Andres pretty much put his finger on the problem here when he mentioned labels on shared objects. You can imagine trying to design this API so that when someone makes reference to label provider xyzzy we look for a function with that name that has some magic return type and call it, similar to what we already do with FDWs and what you recently implemented for TABLESAMPLE. But if the label is on a shared object, in which database shall we call that function? Even for database objects, it won't do at all if the same label provider name is used to mean different things in different databases. Speaking as the guy who came up with much of the design for feature and committed KaiGai's patch implementing it, I have to admit that I didn't think of what problems this would create for pg_upgrade. It seemed to me at the time that this really wasn't that different from something like pg_stat_statements, which also won't work properly unless loaded via shared_preload_libraries. In retrospect, there is a difference, which is that if you don't load pg_stat_statements, your DDL and queries will still execute, but in this case, they won't. That's definitely raising the table stakes, but I'm not sure what to do about it. Preserving shared_preload_libraries across pg_upgrade is a tempting solution, but that isn't guaranteed to solve more problems than it creates. -- 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] CREATE FUNCTION .. LEAKPROOF docs
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/14/2015 03:46 AM, Dean Rasheed wrote: I think the docs for the LEAKPROOF option in create_function.sgml ought to mention RLS as well as security barrier views. Also the current text is no longer strictly correct in light of commit dcbf5948e12aa60b4d6ab65b6445897dfc971e01. Suggested rewording attached. Any objections out there to this doc patch? It rings true and reads well to me. I'd like to check another item off the 9.5 Open Items list.. . - -- Joe Conway -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuWXbAAoJEDfy90M199hlVMkP/RO2Vn8hlslKfP+9ZxgCOQk+ YZPlufD10tvsiWtmjxM/czQVgc4OtzANLS4G9tSL0ICiINWckP8FtF6NLdykic17 inTG3wrU2Q/EH9eIio6RJg6iZ+619A21IrFQwlSOJWB1WlPHHdzUoL2YZJsviEyt 01XTb6P60yg11MENWZGKQzWhL0SgjtWliuI2OVb2UbpE2lHb4KBVyPtnn4LW8SyP a7lJA7WeERAuwlt2C9EbywE1gDJCMOnVnBWHjKtc8fEQKjJGwi6MgXelGBE9QWlx j2TTleHO270m20aXkIaz/LQX1fjpHonWgtwnW11v4IvGHXZLgN89NRdx2rndBs9z lRl0t7DLQ0Fn+h6nB4RQdN8huJD12O8JEYvktYMHPjJrCVKgezxqw2e3xUBdCnU+ 4eo0TrjQQKxzlQvqTc+dnKXWu/xgre6QNjS5AknoKqA6+MXtCQ6OW/uUKQNA8Amc WcxjsIJZaLbqOAaODL9DhdufcCCL1rMWuAWAGH7tJwDeIRJOQDChQp5Dg2YmGU/Y hYAH+bqvJPoo0kAsftgyVocdfp7rDGd3nP87Bg3fFLXobghNIXK1xpXe08IRhAEh wdMT1Np91WNl9wCyOpUiFn9rmP4ZMALofbhofI5hIsj5dTXvMGxbvsDk7xMykujk 3CeKshcZK060TSo1G2Up =9wOA -END PGP SIGNATURE- diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml new file mode 100644 index c5beb16..cc2098c --- a/doc/src/sgml/ref/create_function.sgml +++ b/doc/src/sgml/ref/create_function.sgml @@ -350,9 +350,18 @@ CREATE [ OR REPLACE ] FUNCTION effects. It reveals no information about its arguments other than by its return value. For example, a function which throws an error message for some argument values but not others, or which includes the argument - values in any error message, is not leakproof. The query planner may - push leakproof functions (but not others) into views created with the - literalsecurity_barrier/literal option. See + values in any error message, is not leakproof. This affects how the + system executes queries against views created with the + literalsecurity_barrier/literal option or tables with row level + security enabled. The system will enforce conditions from security + policies and security barrier views before any user-supplied conditions + from the query itself that contain non-leakproof functions, in order to + prevent the inadvertent exposure of data. Functions and operators + marked as leakproof are assumed to be trustworthy, and may be executed + before conditions from security policies and security barrier views. + In addtion, functions which do not take arguments or which are not + passed any arguments from the security barrier view or table do not have + to be marked as leakproof to be executed before security conditions. See xref linkend=sql-createview and xref linkend=rules-privileges. This option can only be set by the superuser. /para -- 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] Supporting TAP tests with MSVC and Windows
On Thu, Jul 30, 2015 at 1:37 AM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/29/2015 10:13 AM, Michael Paquier wrote: On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier michael.paqu...@gmail.com wrote: An updated patch is attached. Attached is v9, that fixes conflicts with 01f6bb4 and recent commits that added TAP tests in pg_basebackup series. Thanks, committed with some more tweaking. Peter just added a slurp_file() function to TestLib.pm, so I used that to replace the call to 'cat' instead of my previous snippet. I also fixed the issue in the pg_basebackup test, that LSN is not necessarily 8 characters long, slightly differently. Calling pg_xlogfile_name seemed a bit indirect. Thanks! -- 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] Eliminating CREATE INDEX comparator TID tie-breaker overhead
On Thu, Jul 23, 2015 at 11:44 AM, Peter Geoghegan p...@heroku.com wrote: If no one weighs in after a few days, I'll mark the patch rejected in the CF app. The patch has been marked rejected in the CF app. I withdraw it. Obviously I still think that the patch is worthwhile, but not if that while is disproportionate to any benefit, which I suspect will be the case if I proceed. -- 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
[HACKERS] Using quicksort and a merge step to significantly improve on tuplesort's single run external sort
The behavior of external sorts that do not require any merge step due to only having one run (what EXPLAIN ANALYZE output shows as an external sort, and not a merge sort) seems like an area that can be significantly improved upon. As noted in code comments, this optimization did not appear in The Art of Computer Programming, Volume III. It's not an unreasonable idea, but it doesn't work well on modern machines due to using heapsort, which is known to use the cache ineffectively. It also often implies significant additional I/O for little or no benefit. I suspect that all the reports we've heard of smaller work_mem sizes improving sort performance are actually down to this one-run optimization *hurting* performance. The existing optimization for this case tries to benefit from avoiding a final N-way merge step; that's what it's all about (this is not to be confused with the other reason why a sort can end in TSS_SORTEDONTAPE -- because random tape access is required, and an *on-the-fly* TSS_FINALMERGE merge step cannot happen. Currently, TSS_SORTEDONTAPE is sometimes the fast case and sometimes the slow case taken only because the caller specified randomAccess, and I'm improving on only the fast case [1], where the caller may or may not have requested randomAccess. I require that they specify !randomAccess to use my improved optimization, though, and I'm not trying to avoid a merge step.) The existing optimization just dumps all tuples in the memtuples array (which is a heap at that point) to tape, from the top of the heap, writing a tuple out at a time, maintaining the heap invariant throughout. Then, with the memtuples array emptied, tuples are fetched from tape/disk for client code, without any merge step occurring on-the-fly (or at all). Patch -- quicksort with spillover = With the attached patch, I propose to add an additional, better one run special case optimization. This new special case splits the single run into 2 subruns. One of the runs is comprised of whatever tuples were in memory when the caller finished passing tuples to tuplesort. To sort that, we use quicksort, which in general has various properties that make it much faster than heapsort -- it's a cache oblivious algorithm, which is important these days. The other subrun is whatever tuples were on-tape when tuplesort_performsort() was called. This will often be a minority of the total, but certainly never much more than half. This is already sorted when tuplesort_performsort() is reached. This spillover is already inserted at the front of the sorted-on-tape tuples, and so already has reasonably good cache characteristics. With the patch, we perform an on-the-fly merge that is somewhat similar to the existing (unaffected) merge sort TSS_FINALMERGE case, except that one of the runs is in memory, and is potentially much larger than the on-tape/disk run (but never much smaller), and is quicksorted. The existing merge sort case similarly is only used when the caller specified !randomAccess. For subtle reasons, the new TSS_MEMTAPEMERGE case will happen significantly more frequently than the existing, comparable TSS_SORTEDONTAPE case currently happens (this applies to !randomAccess callers only, of course). See comments added to tuplesort_performsort(), and the commit message for the details. Note that the existing, comparable case was relocated to tuplesort_performsort(), to highlight that it is now a fallback for the new TSS_MEMTAPEMERGE case (also in tuplesort_performsort()). Performance == This new approach can be much faster. For example: select setseed(1); -- 10 million tuple table with low cardinality integer column to sort: create unlogged table low_cardinality_int4 as select (random() * 1000)::int4 s, 'abcdefghijlmn'::text junk from generate_series(1, 1000); set work_mem = '225MB'; -- test query: select count(distinct(s)) from low_cardinality_int4; count --- 1001 (1 row) On my laptop, a patched Postgres takes about 4.2 seconds to execute this query. Master takes about 16 seconds. The patch sees this case quicksort 9,830,398 tuples out of a total of 10 million with this 225MB work_mem setting. This is chosen to be a sympathetic case, but it is still quite realistic. We should look at a much less sympathetic case, too. Even when the in-memory subrun is about as small as it can be while still having this new special case optimization occur at all, the patch still ends up pretty far ahead of the master branch. With work_mem at 100MB, 4,369,064 tuples are quicksorted by the patch when the above query is executed, which is less than half of the total (10 million). Execution with the patch now takes about 10.2 seconds. Master is about 14.7 seconds with the same work_mem setting (yes, master gets faster as the patch gets slower as work_mem is reduced...but they never come close to meeting). That's still a fairly decent improvement, and it occurs when we're on the verge of not using the
Re: [HACKERS] dblink: add polymorphic functions.
I wrote: Well, it would depend on how we fixed %TYPE, but my thought is that we should teach the core parser to accept variable%TYPE anywhere that a suitable variable is in scope. The core already allows related syntaxes in some utility commands, but not within DML commands. I poked at this a little bit, and concluded that it's probably impossible to do it exactly like that, at least not in a backward-compatible way. The difficulty is that TYPE is an unreserved keyword, and can therefore be a column name, while of course '%' is a valid operator. So for example SELECT x::int%type FROM ... currently means cast column x to integer and then modulo it by column type. AFAICS there's no way to introduce %TYPE into the :: cast syntax without breaking this interpretation. I suppose that we could dodge this ambiguity by allowing %TYPE only in the CAST() notation, but this would be the first time that CAST and :: had any differences in how the type name could be spelled, and that's not a nice inconsistency to introduce either. What's possibly more palatable is to introduce some other special notation for obtain the type of this expression at parse time. I'm thinking for example about SELECT x::pg_typeof(some_expression) FROM ... Maybe it would be too confusing to overload pg_typeof this way, in which case we could choose some other name. Aside from that consideration, this approach would have the effect of preventing pg_typeof from being used as an actual type name, or at least from being used as a type name that can have typmod, but that doesn't seem like a huge drawback. This way would have the rather nice property that some_expression could actually be any parseable expression, not merely a qualified variable name (which I think is the only case that we'd have had any hope of supporting with %TYPE). 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] dblink: add polymorphic functions.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2015 05:13 PM, Tom Lane wrote: What's possibly more palatable is to introduce some other special notation for obtain the type of this expression at parse time. I'm thinking for example about SELECT x::pg_typeof(some_expression) FROM ... Maybe it would be too confusing to overload pg_typeof this way, in which case we could choose some other name. Aside from that consideration, this approach would have the effect of preventing pg_typeof from being used as an actual type name, or at least from being used as a type name that can have typmod, but that doesn't seem like a huge drawback. This way would have the rather nice property that some_expression could actually be any parseable expression, not merely a qualified variable name (which I think is the only case that we'd have had any hope of supporting with %TYPE). You think this could be made to work? SELECT x::TYPE OF(some_expression) FROM ... Then we would also have: SELECT CAST(x AS TYPE OF(some_expression)) FROM ... Seems nicer than pg_typeof - -- Joe Conway -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuW3GAAoJEDfy90M199hlY6UP/026qCp5sxSz8zsnY9FyqqMp Xf+a1nlqZFB821zQtMx7NtKtxjiC45jqbPSerLqCSbbpMftLG/iy5+/wdWJqAIoK 283Q23hD6r7hPwR2nown3BH5F1AFGCppG5KWebOgl01jVQSWBfFiRLrImFi/2FVs sp3fHFmONp2kIYoAZwyFyZXv2n4D9Qhp0tIq94Dz0LGaszsfpYObCapPkgwAgaYZ TSk5FAmD+IIJsNadhuk6IfJRObY5DgLL5keJSiuNs4Xpixq72KjqgnYQeXqgoT6U AOqEkyg/KejkBSmuZRtHc9qnewGPzQn9TBXZEGPF+/ifpHC7+gL2au97kUW35j5l 3Ox57hTTRgr3oRvrEkGN/1uM/yDiobXb2HOp70mIeuYAWp3juwfxZQybRMYCoM8a O/oyJqTSX2Dn/GkzeAOxbaQJulMeJfkivnwak0BhdaX3d4bDJz1ylNgz/B4Gtcnn x4FVdTEfTBGFKFmfyDB5iAvpRrDCCDYcd2KcHA1J34vm8Ccm6m3aauJoi4zqsubh bQnia2aoIAtnIOei/zVaST7+G+B3WAod3MDmrctjkx1lACTIeQVHq7q/A3iUPwcF 7Lfu9vr/6IBsAlvkdsX7zNZsIzAlov+ObrKZHBxeq2lB31MH1jeulX8lx+131+aI ATx7kmeBlB60dkccMEhv =VgLe -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/01/2015 02:21 AM, Dean Rasheed wrote: While going through this, I spotted another issue --- in a DML query with additional non-target relations, such as UPDATE t1 .. FROM t2 .., the old code was checking the UPDATE policies of both t1 and t2, but really I think it ought to be checking the SELECT policies of t2 (in the same way as this query requires SELECT table permissions on t2, not UPDATE permissions). I've changed that and added new regression tests to test that change. I assume the entire refactoring patch needs a fair bit of work to rebase against current HEAD, but I picked out the attached to address just the above issue. Does this look correct, and if so does it make sense to apply at least this part right now? Thanks, Joe -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuXFuAAoJEDfy90M199hlOMkP/RZoBU0MJF64GjHfuLVa3T5w PnDrnIoLBMOgOzkXrI+Ov0w0ESXltNvYjyIxkuyaB5PuoeDOdyYnnzbpPe7hH4pv zDjSinJnfNmFEcm0UjBzuBiSH0dv52PEIToexTKu6SnjvH3Co/Hk4Ar2uZ0r7bRF krl+Dd4kZX1uuRIigsSFqy873C79m3o11Szs5aW8c5od9adGxSvRHqZNf/UIDIbZ CxAo0E3Tlw0DmGl1cw1tdN8gWMzmvx5dQ0ih3+0/hkVUqN9p3Pg8BGajSvxxFlb2 l4+6RZGUw5ZTpJxRZf/zPc4updhq0zf/Z5g7GUYddrhO29eLS6al2ySlb7HL5G9M VPMjEzXuhFwhxSMdgHfz8UQh82KuNENMTKp81BvtIgZ7w86A9lWrKxCLaVMGhi8m MnfCQ4cdOmnE2vEHi0Ue3Dg/rvYO8QpVW0JKDOdzoqPErC7to9vorXI5X3vcfLbF fYfiJe6wUwbDEdxh/z0oKVFxWlf2NRk4pd+jZ7C+ibraLIwgEgZe7G4GEje5LxSP h4zTfx0sj3IyrvqziurO/aZqIBXBZEsm3Gv6OQs26C5Xvkx/QmgROB42lHwcDYri BTk6+uzNYKbX+kW56vEY0f0WMTLYZzc6jZRIVr3s+aLeyG0P9acY/n3uY1xBFCZZ Xb7gmepAN3rY1CF7By9o =e5hz -END PGP SIGNATURE- diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index 2386cf0..562dbc9 100644 *** a/src/backend/rewrite/rowsecurity.c --- b/src/backend/rewrite/rowsecurity.c *** get_row_security_policies(Query *root, C *** 147,154 return; } ! /* Grab the built-in policies which should be applied to this relation. */ rel = heap_open(rte-relid, NoLock); rowsec_policies = pull_row_security_policies(commandType, rel, user_id); --- 147,164 return; } ! /* ! * RLS is enabled for this relation. ! * ! * Get the security policies that should be applied, based on the command ! * type. Note that if this isn't the target relation, we actually want ! * the relation's SELECT policies, regardless of the query command type, ! * for example in UPDATE t1 ... FROM t2 we need to apply t1's UPDATE ! * policies and t2's SELECT policies. ! */ rel = heap_open(rte-relid, NoLock); + if (rt_index != root-resultRelation) + commandType = CMD_SELECT; rowsec_policies = pull_row_security_policies(commandType, rel, user_id); diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index b0556c2..6fc80af 100644 *** a/src/test/regress/expected/rowsecurity.out --- b/src/test/regress/expected/rowsecurity.out *** CREATE POLICY p ON t USING (max(c)); -- *** 3033,3038 --- 3033,3121 ERROR: aggregate functions are not allowed in policy expressions ROLLBACK; -- + -- Non-target relations are only subject to SELECT policies + -- + SET SESSION AUTHORIZATION rls_regress_user0; + CREATE TABLE r1 (a int); + CREATE TABLE r2 (a int); + INSERT INTO r1 VALUES (10), (20); + INSERT INTO r2 VALUES (10), (20); + GRANT ALL ON r1, r2 TO rls_regress_user1; + CREATE POLICY p1 ON r1 USING (true); + ALTER TABLE r1 ENABLE ROW LEVEL SECURITY; + CREATE POLICY p1 ON r2 FOR SELECT USING (true); + CREATE POLICY p2 ON r2 FOR INSERT WITH CHECK (false); + CREATE POLICY p3 ON r2 FOR UPDATE USING (false); + CREATE POLICY p4 ON r2 FOR DELETE USING (false); + ALTER TABLE r2 ENABLE ROW LEVEL SECURITY; + SET SESSION AUTHORIZATION rls_regress_user1; + SELECT * FROM r1; + a + + 10 + 20 + (2 rows) + + SELECT * FROM r2; + a + + 10 + 20 + (2 rows) + + -- r2 is read-only + INSERT INTO r2 VALUES (2); -- Not allowed + ERROR: new row violates row level security policy for r2 + UPDATE r2 SET a = 2 RETURNING *; -- Updates nothing + a + --- + (0 rows) + + DELETE FROM r2 RETURNING *; -- Deletes nothing + a + --- + (0 rows) + + -- r2 can be used as a non-target relation in DML + INSERT INTO r1 SELECT a + 1 FROM r2 RETURNING *; -- OK + a + + 11 + 21 + (2 rows) + + UPDATE r1 SET a = r2.a + 2 FROM r2 WHERE r1.a = r2.a RETURNING *; -- OK + a | a + + + 12 | 10 + 22 | 20 + (2 rows) + + DELETE FROM r1 USING r2 WHERE r1.a = r2.a + 2 RETURNING *; -- OK + a | a + + + 12 | 10 + 22 | 20 + (2 rows) + + SELECT * FROM r1; + a + + 11 + 21 + (2 rows) + + SELECT * FROM r2; + a + + 10 + 20 + (2 rows) + + SET SESSION AUTHORIZATION rls_regress_user0; + DROP TABLE r1; + DROP TABLE r2; + -- -- Clean up objects -- RESET SESSION AUTHORIZATION; diff --git a/src/test/regress/sql/rowsecurity.sql
Re: [HACKERS] dblink: add polymorphic functions.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2015 07:58 PM, Tom Lane wrote: We can definitely do SELECT x::any_single_unreserved_word(some_expression) FROM ... because that's actually not something the grammar needs to distinguish from type-with-a-typmod; we can deal with the special case in LookupTypeName. It's just a matter of picking a word people like. What about just TYPE then? SELECT x::TYPE(some_expression) FROM ... SELECT CAST (x AS TYPE(some_expression)) FROM ... - -- Joe Conway -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuar5AAoJEDfy90M199hlN8gP/i2IdtOsJ1PasKfUjAegbHf5 HIWLI7lZw2OMb451zrrNJbfTk1xY+OUJX8tRTLku8GyoZ9FrhDnBo0JuZuHMQOo4 ulWPH7JYGQVb89FYANNbubIehfJ0Y5TCr/ihkpmeVR6sTR3OZDSvdVtymF34wfZE 96i2S6QqWHN4V6hNXjTuzaIu4BXFXvZg3N9yNvBRrpnif53jfKPnca6wSeHJgTWv w8L6mKQbLDW+5azVmuFX/1PyLxMRphsZL6G4+yyASkzQP2VOGDRQrM4Uavoot9Ja l1Ez4bBoK3ERxfovnSWfwlsqhKmQ41TijoIu/Ex/s1O3dL2LVQ2qBp8cCl8pX9zq Fnk11ueAvjkVt8/mIQFkGY+noes8vqWGe6yB0FYXjJvFfL4DXgfmthdyyCGJM1l9 JLI034tkflXKEkk5Ty9gOeAaMzqztqmIRYoQKK7O18DOKNH3Fgoa5Vh2Fz/iJI6G rjQtfcZwv6ukN0qyQ8QB42CvLJVQ5KVwdTSr/93eCipSIuTPJNEoIBSh7H02WN7Q fqQcKsM9m9ZTkAYP9uQCMEwusiKoPZt41Tdwf5fbhuOHoSim2Tab63eMEoUkRsqu Bgqql/U5/MRsoAoDp4ALr2LbugnnTVNhrqrP58e45yl+694UEyh9XRpZmWUpX9Lw k+qPyOJCnLBwOcmS0tv1 =+T37 -END PGP SIGNATURE- -- 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] The real reason why TAP testing isn't ready for prime time
Michael Paquier michael.paqu...@gmail.com writes: hamster has not complained for a couple of weeks now, and the issue was reproducible every 4~6 days: http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=hamsterbr=HEAD Hence let's consider the issue as resolved. Nah, I'm afraid not. We are definitely still seeing a lot of unexplained pg_ctl failures; as a recent example, http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=axolotldt=2015-07-29%2018%3A00%3A19 Now maybe that's not the same thing as hamster's issue, but there's still something funny going on. That's why I've been bitching about the lack of debug information. I hope that we can make progress on understanding this once Andrew pushes out the new buildfarm release (especially since some of his machines are particularly prone to this type of failure, so I trust he'll update those critters promptly). 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] Improving test coverage of extensions with pg_dump
On Thu, Jul 30, 2015 at 5:54 AM, Andreas Karlsson andr...@proxel.se wrote: What I do not like though is how the path src/test/tables_fk/t/ tells us nothing about what features are of PostgreSQL are tested here. For this I personally prefer the earlier versions where I think that was clear. +Simple module used to check data dump ordering of pg_dump with tables +linked with foreign keys. The README mentions that this is to test pg_dump, perhaps referring to the TAP tests makes sense as well? Another thought: would it be worthwhile to also add an assertion to check if the data really was restored properly or would that just be redundant code? That seems worth doing, hence added something for it. Thanks for the suggestion. At the same time I have added an entry in .gitignore for the generated tmp_check/. Regards, -- Michael From 26d507360aa8780ca51884fe3a8d82e5ae967481 Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Thu, 30 Jul 2015 11:46:54 +0900 Subject: [PATCH] Add TAP test for pg_dump checking data dump of extension tables The test added checks the data dump ordering of tables added in an extension linked among them with foreign keys. In order to do that, a test extension in the set of TAP tests of pg_dump, combined with a TAP test script making use of it. --- src/test/modules/Makefile | 1 + src/test/modules/tables_fk/.gitignore | 1 + src/test/modules/tables_fk/Makefile | 24 + src/test/modules/tables_fk/README | 5 src/test/modules/tables_fk/t/001_dump_test.pl | 39 +++ src/test/modules/tables_fk/tables_fk--1.0.sql | 20 ++ src/test/modules/tables_fk/tables_fk.control | 5 7 files changed, 95 insertions(+) create mode 100644 src/test/modules/tables_fk/.gitignore create mode 100644 src/test/modules/tables_fk/Makefile create mode 100644 src/test/modules/tables_fk/README create mode 100644 src/test/modules/tables_fk/t/001_dump_test.pl create mode 100644 src/test/modules/tables_fk/tables_fk--1.0.sql create mode 100644 src/test/modules/tables_fk/tables_fk.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 8213e23..683e9cb 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global SUBDIRS = \ commit_ts \ dummy_seclabel \ + tables_fk \ test_ddl_deparse \ test_parser \ test_rls_hooks \ diff --git a/src/test/modules/tables_fk/.gitignore b/src/test/modules/tables_fk/.gitignore new file mode 100644 index 000..b6a2a01 --- /dev/null +++ b/src/test/modules/tables_fk/.gitignore @@ -0,0 +1 @@ +/tmp_check/ diff --git a/src/test/modules/tables_fk/Makefile b/src/test/modules/tables_fk/Makefile new file mode 100644 index 000..d018517 --- /dev/null +++ b/src/test/modules/tables_fk/Makefile @@ -0,0 +1,24 @@ +# src/test/modules/tables_fk/Makefile + +EXTENSION = tables_fk +DATA = tables_fk--1.0.sql +PGFILEDESC = tables_fk - set of dumpable tables linked by foreign keys + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/tables_fk +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif + +check: all + $(prove_check) + +installcheck: install + $(prove_installcheck) + +temp-install: EXTRA_INSTALL=src/test/modules/tables_fk diff --git a/src/test/modules/tables_fk/README b/src/test/modules/tables_fk/README new file mode 100644 index 000..049b75c --- /dev/null +++ b/src/test/modules/tables_fk/README @@ -0,0 +1,5 @@ +tables_fk += + +Simple module used to check data dump ordering of pg_dump with tables +linked with foreign keys using TAP tests. diff --git a/src/test/modules/tables_fk/t/001_dump_test.pl b/src/test/modules/tables_fk/t/001_dump_test.pl new file mode 100644 index 000..9d3d5ff --- /dev/null +++ b/src/test/modules/tables_fk/t/001_dump_test.pl @@ -0,0 +1,39 @@ +use strict; +use warnings; +use TestLib; +use Test::More tests = 4; + +my $tempdir = tempdir; + +start_test_server $tempdir; + +psql 'postgres', 'CREATE EXTENSION tables_fk'; + +# Insert some data before running the dump, this is needed to check +# consistent data dump of tables with foreign key dependencies +psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)'; +psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)'; +psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)'; + +# Create a table depending on a FK defined in the extension +psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id))'; +psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)'; + +# Take a dump then re-deploy it to a new database +command_ok(['pg_dump', '-d', 'postgres', '-f', $tempdir/dump.sql], + 'taken dump with installed extension'); +command_ok(['createdb', 'foobar1'], 'created new database to redeploy
Re: [HACKERS] dblink: add polymorphic functions.
Joe Conway m...@joeconway.com writes:
On 07/29/2015 05:13 PM, Tom Lane wrote:
What's possibly more palatable is to introduce some other special
notation for obtain the type of this expression at parse time.
I'm thinking for example about
SELECT x::pg_typeof(some_expression) FROM ...
You think this could be made to work?
SELECT x::TYPE OF(some_expression) FROM ...
Hmmm ... that looks kind of nice, but a quick experiment with
bison says it's ambiguous. I tried this just as proof-of-concept:
*** src/backend/parser/gram.y~ Fri Jul 24 21:40:02 2015
--- src/backend/parser/gram.y Wed Jul 29 22:45:04 2015
*** GenericType:
*** 11065,11070
--- 11065,11074
$$-typmods = $3;
$$-location = @1;
}
+ | TYPE_P OF '(' a_expr ')'
+ {
+ $$ =
makeTypeNameFromNameList(lcons(makeString($1), $2));
+ }
;
opt_type_modifiers: '(' expr_list ')' { $$ = $2; }
and got a shift/reduce conflict. I'm not quite sure why, but since OF
is also not a reserved keyword, it's likely that this is unfixable.
In fact, I also tried TYPE_P FROM, not because that is especially
great syntax but because FROM *is* fully reserved, and that did not
work either. So this isn't looking like a promising line of thought.
We can definitely do
SELECT x::any_single_unreserved_word(some_expression) FROM ...
because that's actually not something the grammar needs to distinguish
from type-with-a-typmod; we can deal with the special case in
LookupTypeName. It's just a matter of picking a word people like.
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] The real reason why TAP testing isn't ready for prime time
On Sun, Jun 21, 2015 at 7:06 AM, Michael Paquier michael.paqu...@gmail.com wrote: And, we get a failure: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamsterdt=2015-06-20%2017%3A59%3A01 I am not sure why buildfarm runs makes it more easily reproducible, one of the reasons may be the perl scripts run underground. hamster has not complained for a couple of weeks now, and the issue was reproducible every 4~6 days: http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=hamsterbr=HEAD Hence let's consider the issue as resolved. -- 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] Division by zero in planner.c:grouping_planner()
Qingqing Zhou zhouqq.postg...@gmail.com writes: On Wed, Jul 29, 2015 at 11:26 AM, Piotr Stefaniak postg...@piotr-stefaniak.me wrote: + Assert(path_rows != 0); if (tuple_fraction = 1.0) tuple_fraction /= path_rows; } This does not sounds right: path_rows only used when tuple_fractions = 1.0. So the new Assert is an overkill. Well, the point is that we'll get a zero-divide if path_rows is zero. It looks to me like the planner has managed to prove that the result is empty (because a=3 contradicts a5), so it's marked the final_rel as dummy, which includes setting its rows estimate to zero. In most situations relation rows estimates aren't allowed to be zero, which is how come this code isn't expecting the case ... but it needs to cope with 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
[HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
Hi, Heikki complained about pg_receivexlog --create-slot starting to stream in his talk in St. Petersburg. Saying that that makes it a hard to script feature - and I have to agree. Since that option is new to 9.5 we can should change that behaviour now if we decide to. Michael, what do you think? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On Wed, Jul 29, 2015 at 4:51 PM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/29/2015 10:37 AM, Andres Freund wrote: Heikki complained about pg_receivexlog --create-slot starting to stream in his talk in St. Petersburg. Saying that that makes it a hard to script feature - and I have to agree. Since that option is new to 9.5 we can should change that behaviour now if we decide to. To be clear, I think pg_receivexlog --create-slot should only create the slot, and exit. Even if I would like to make pg_recvlogical and pg_receivexlog behave as similarly as possible by having an additional switch --start in pg_receivexlog to control if it starts to stream or not, this ship has already sailed for backward-compatibility's sake... Then let's just create the slot and exit(). -- 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] Typo in comment in ATPrepChangePersistence
On 07/29/2015 05:26 AM, Amit Langote wrote: Attached fixes a typo: - * no permanent tables cannot reference unlogged ones. + * permanent tables cannot reference unlogged ones. Thanks, fixed. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On 07/29/2015 10:58 AM, Michael Paquier wrote: On Wed, Jul 29, 2015 at 4:51 PM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/29/2015 10:37 AM, Andres Freund wrote: Heikki complained about pg_receivexlog --create-slot starting to stream in his talk in St. Petersburg. Saying that that makes it a hard to script feature - and I have to agree. Since that option is new to 9.5 we can should change that behaviour now if we decide to. To be clear, I think pg_receivexlog --create-slot should only create the slot, and exit. Even if I would like to make pg_recvlogical and pg_receivexlog behave as similarly as possible by having an additional switch --start in pg_receivexlog to control if it starts to stream or not, this ship has already sailed for backward-compatibility's sake... Then let's just create the slot and exit(). Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone will have it integrated into production scripts etc. So maybe we could just change it. I'm not sure I understand the proposal though. If you don't specify --create-slot, nor --start, what would the program do? Nothing? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 02:36, Joe Conway joe.con...@crunchydata.com wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but CreatePolicy() and DropPolicy() lack their respective hook invocations. Patch attached. Actually AlterPolicy() was also missing its hook -- the existing InvokeObjectPostAlterHook() was only in rename_policy(). I'm not 100% sure about the hook placement -- would appreciate if someone could confirm I got it correct. The CreatePolicy() and AlterPolicy() changes look OK to me, but the RemovePolicyById() change looks to be unnecessary --- RemovePolicyById() is called only from doDeletion(), which in turned is called only from deleteOneObject(), which already invokes the drop hooks. So ISTM that RemovePolicyById() doesn't need to do anything, right? Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 2015-07-29 08:57:38 +0100, Simon Riggs wrote: On 22 July 2015 at 05:43, Michael Paquier michael.paqu...@gmail.com wrote: Now, do we plan to do something about the creation of a slot. I imagine that it would be useful if we could have --create-slot to create a slot when beginning a base backup and if-not-exists to control the error flow. There is already CreateReplicationSlot for this purpose in streamutils.c so most of the work is already done. Also, when a base backup fails for a reason or another, we should try to drop the slot in disconnect_and_exit() if it has been created by pg_basebackup. if if-not-exists is true and the slot already existed when beginning, we had better not dropping it perhaps... What is the purpose of creating a temporary slot? If we create a slot when one is needed and then drop automatically on session disconnect (as Heikki suggest), what benefit does using a slot give us? The goal is to have a reliable pg_basebackup that doesn't fail due to missing WAL (which can easily happen even with -X) . That seems like a sane desire. The point of using a temporary slot is to not have a leftover slot afterwards, reserving resources. Especially important if the basebackup actually failed... -- 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] dblink: add polymorphic functions.
On 07/18/2015 01:32 AM, Corey Huinker wrote: So this patch would result in less C code while still adding 3 new functions. Can anyone think of why that wouldn't be the best way to go? Let's pursue the CAST(srf() AS row_rtype) syntax that Joe suggested upthread (http://www.postgresql.org/message-id/559a9643.9070...@joeconway.com). For some reason, the discussion went on around the details of the submitted patch after that, even though everyone seemed to prefer the CAST() syntax. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 29 July 2015 at 09:09, Andres Freund and...@anarazel.de wrote: On 2015-07-29 08:57:38 +0100, Simon Riggs wrote: On 22 July 2015 at 05:43, Michael Paquier michael.paqu...@gmail.com wrote: Now, do we plan to do something about the creation of a slot. I imagine that it would be useful if we could have --create-slot to create a slot when beginning a base backup and if-not-exists to control the error flow. There is already CreateReplicationSlot for this purpose in streamutils.c so most of the work is already done. Also, when a base backup fails for a reason or another, we should try to drop the slot in disconnect_and_exit() if it has been created by pg_basebackup. if if-not-exists is true and the slot already existed when beginning, we had better not dropping it perhaps... What is the purpose of creating a temporary slot? If we create a slot when one is needed and then drop automatically on session disconnect (as Heikki suggest), what benefit does using a slot give us? The goal is to have a reliable pg_basebackup that doesn't fail due to missing WAL (which can easily happen even with -X) . That seems like a sane desire. Agreed, that is sane. Slots are good. The point of using a temporary slot is to not have a leftover slot afterwards, reserving resources. Especially important if the basebackup actually failed... Creating a slot and then deleting it if the session disconnects does not successfully provide the functionality desired above. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On 29 July 2015 at 08:37, Andres Freund and...@anarazel.de wrote: Heikki complained about pg_receivexlog --create-slot starting to stream in his talk in St. Petersburg. Saying that that makes it a hard to script feature - and I have to agree. Since that option is new to 9.5 we can should change that behaviour now if we decide to. Michael, what do you think? --drop-slot seems pointless, since you can just do that with psql If we make --create-slot do nothing but add the slot, then that seems pointless also Would we need to add those options to all commands, when it can be done with psql? I'd suggest just removing --create-slot and --drop-slot altogether -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On 2015-07-29 08:54:40 +0100, Simon Riggs wrote: --drop-slot seems pointless, since you can just do that with psql If we make --create-slot do nothing but add the slot, then that seems pointless also Would we need to add those options to all commands, when it can be done with psql? They work over the replication protocol, which is handy, because it can be done with the same user/permissions as the normal pg_receivexlog. -- 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] Don'st start streaming after creating a slot in pg_receivexlog
On 29 July 2015 at 09:01, Andres Freund and...@anarazel.de wrote: On 2015-07-29 08:54:40 +0100, Simon Riggs wrote: --drop-slot seems pointless, since you can just do that with psql If we make --create-slot do nothing but add the slot, then that seems pointless also Would we need to add those options to all commands, when it can be done with psql? They work over the replication protocol, which is handy, because it can be done with the same user/permissions as the normal pg_receivexlog. It's useful to separate permissions for creating/dropping a slot from using it. A one-time act configures (or re-configures) how you wish the cluster to look, another more regular task is to connect to the slot and use it. But if you want to have a single user with privileges for both, you can create that. I don't see it as something that we need to support in the replication protocol, since that would require us to extend it every time we think of something else. Creating a temporary slot goes against the whole concept of slots, so using the same id in the same script isn't actually needed, except maybe to simplify testing. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 05:02, Joe Conway joe.con...@crunchydata.com wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (7) Using an aggregate function in a policy predicate elicits an inapposite error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new ParseExprKind. Test case: Patch attached. Comments? I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. There are a couple of places in test_rls_hooks.c that also need updating. I think that check_agglevels_and_constraints() and transformWindowFuncCall() could be made to emit more targeted error messages for EXPR_KIND_POLICY, for example aggregate functions are not allowed in policy USING and WITH CHECK expressions. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On Wed, Jul 29, 2015 at 10:02 AM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/29/2015 10:58 AM, Michael Paquier wrote: On Wed, Jul 29, 2015 at 4:51 PM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/29/2015 10:37 AM, Andres Freund wrote: Heikki complained about pg_receivexlog --create-slot starting to stream in his talk in St. Petersburg. Saying that that makes it a hard to script feature - and I have to agree. Since that option is new to 9.5 we can should change that behaviour now if we decide to. To be clear, I think pg_receivexlog --create-slot should only create the slot, and exit. Even if I would like to make pg_recvlogical and pg_receivexlog behave as similarly as possible by having an additional switch --start in pg_receivexlog to control if it starts to stream or not, this ship has already sailed for backward-compatibility's sake... Then let's just create the slot and exit(). Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone will have it integrated into production scripts etc. So maybe we could just change it. I'm not sure I understand the proposal though. If you don't specify --create-slot, nor --start, what would the program do? Nothing? Maybe we should make --start an optional flag for --create-slot in both pg_recvlogical and pg_receivexlog? So that if you didn't use --create-slot or --drop-slot, then it is assumed that you want to start streaming. And if you're using --create-slot, you can still start streaming right away, though not by default. At the moment I find it odd that pg_recvlogical -S myslot doesn't start streaming and require you to use --start explicitly. -- *Oleksandr Shulgin* *Database Engineer* Mobile: +49 160 84-90-639 Email: oleksandr.shul...@zalando.de
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On 07/29/2015 10:37 AM, Andres Freund wrote: Heikki complained about pg_receivexlog --create-slot starting to stream in his talk in St. Petersburg. Saying that that makes it a hard to script feature - and I have to agree. Since that option is new to 9.5 we can should change that behaviour now if we decide to. To be clear, I think pg_receivexlog --create-slot should only create the slot, and exit. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Supporting TAP tests with MSVC and Windows
On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier michael.paqu...@gmail.com wrote: An updated patch is attached. Attached is v9, that fixes conflicts with 01f6bb4 and recent commits that added TAP tests in pg_basebackup series. -- Michael 0001-TAP-tests-for-MSVC.patch Description: binary/octet-stream -- 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] Don'st start streaming after creating a slot in pg_receivexlog
On Wed, Jul 29, 2015 at 5:02 PM, Heikki Linnakangas wrote: Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone will have it integrated into production scripts etc. So maybe we could just change it. This sounds good to me as well. I'm not sure I understand the proposal though. In short, I would propose the following: - Have --create-slot only create a slot, then exit for both pg_recvlogical and pg_receivexlog. - Have --drop-slot drop a slot, then exit. - Remove the --start switch in pg_recvlogical, and let the utility start streaming by default. By doing so both utilities will behave similarly with the same set of options. If you don't specify --create-slot, nor --start, what would the program do? Nothing? Complain if neither --create-slot nor --drop-slot nor --start are specified: $ pg_recvlogical -f - --slot toto -d postgres pg_recvlogical: at least one action needs to be specified Try pg_recvlogical --help for more information. $ echo $? 1 -- 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] LWLock deadlock and gdb advice
On 07/29/2015 02:39 PM, Andres Freund wrote: On 2015-07-15 18:44:03 +0300, Heikki Linnakangas wrote: Previously, LWLockAcquireWithVar set the variable associated with the lock atomically with acquiring it. Before the lwlock-scalability changes, that was straightforward because you held the spinlock anyway, but it's a lot harder/expensive now. So I changed the way acquiring a lock with a variable works. There is now a separate flag, LW_FLAG_VAR_SET, which indicates that the current lock holder has updated the variable. The LWLockAcquireWithVar function is gone - you now just use LWLockAcquire(), which always clears the LW_FLAG_VAR_SET flag, and you can call LWLockUpdateVar() after that if you want to set the variable immediately. This passes make check, but I haven't done any testing beyond that. Does this look sane to you? The prime thing I dislike about the patch is how long it now holds the spinlock in WaitForVar. I don't understand why that's necessary? There's no need to hold a spinlock until after the mustwait = (pg_atomic_read_u32(lock-state) LW_VAL_EXCLUSIVE) != 0; unless I miss something? In an earlier email you say: After the spinlock is released above, but before the LWLockQueueSelf() call, it's possible that another backend comes in, acquires the lock, changes the variable's value, and releases the lock again. In 9.4, the spinlock was not released until the process was queued. But that's not a problem. The updater in that case will have queued a wakeup for all waiters, including WaitForVar()? If you release the spinlock before LWLockQueueSelf(), then no. It's possible that the backend you wanted to wait for updates the variable's value before you've queued up. Or even releases the lock, and it gets re-acquired by another backend, before you've queued up. Or are you thinking of just moving the SpinLockRelease to after LWLockQueueSelf(), i.e. to the other side of the mustwait = ... line? That'd probably be ok. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On 2015-07-29 09:23:43 +0100, Simon Riggs wrote: Creating a temporary slot goes against the whole concept of slots, so using the same id in the same script isn't actually needed, except maybe to simplify testing. The concept of a slot is to reserve resources. I don't see why it's wrong to reserve resources temporarily. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 29 July 2015 at 11:43, Andres Freund and...@anarazel.de wrote: On 2015-07-29 09:17:04 +0100, Simon Riggs wrote: On 29 July 2015 at 09:09, Andres Freund and...@anarazel.de wrote: The point of using a temporary slot is to not have a leftover slot afterwards, reserving resources. Especially important if the basebackup actually failed... Creating a slot and then deleting it if the session disconnects does not successfully provide the functionality desired above. Uh? If you create the slot, start streaming, and then start the basebackup, it does. It does *not* guarantee that the base backup can be converted into a replica, but it's sufficient to guarantee it can brought out of recovery. Perhaps we are misunderstanding the word it here. it can be brought out of recovery? You appear to be saying that a backup that disconnects before completion is useful in some way. How so? If the slot is cleaned up on disconnect, as suggested, then you end up with half a backup and WAL is cleaned up. The only possible use for slots is to reserve resources (as you say); the resources will clearly not be reserved if we drop the slot on disconnect. What use is that? -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] Support for N synchronous standby servers - take 2
On Tue, Jul 21, 2015 at 3:50 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Mon, Jul 20, 2015 at 9:59 PM, Beena Emerson memissemer...@gmail.com
wrote:
Simon Riggs wrote:
The choice between formats is not
solely predicated on whether we have multi-line support.
I still think writing down some actual use cases would help bring the
discussion to a conclusion. Inventing a general facility is hard without
some clear goals about what we need to support.
We need to at least support the following:
- Grouping: Specify of standbys along with the minimum number of commits
required from the group.
- Group Type: Groups can either be priority or quorum group.
As far as I understood at the lowest level a group is just an alias
for a list of nodes, quorum or priority are properties that can be
applied to a group of nodes when this group is used in the expression
to define what means synchronous commit.
- Group names: to simplify status reporting
- Nesting: At least 2 levels of nesting
If I am following correctly, at the first level there is the
definition of the top level objects, like groups and sync expression.
The grouping and using same application_name different server is similar.
How does the same application_name different server work?
Using JSON, sync rep parameter to replicate in 2 different clusters could be
written as:
{remotes:
{quorum: 2,
servers: [{london:
{priority: 2,
servers: [lndn1, lndn2, lndn3]
}}
,
{nyc:
{priority: 1,
servers: [ny1, ny2]
}}
]
}
}
The same parameter in the new language (as suggested above) could be written
as:
'remotes: 2(london: 1[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
OK, there is a typo. That's actually 2(london: 2[lndn1, lndn2, lndn3],
nyc: 1[ny1, ny2]) in your grammar. Honestly, if we want group aliases,
I think that JSON makes the most sense. One of the advantage of a
group is that you can use it in several places in the blob and set
different properties into it, hence we should be able to define a
group out of the sync expression.
Hence I would think that something like that makes more sense:
{
sync_standby_names:
{
quorum:2,
nodes:
[
{priority:1,group:cluster1},
{quorum:2,nodes:[node1,node2,node3]}
]
},
groups:
{
cluster1:[node11,node12,node13],
cluster2:[node21,node22,node23]
}
}
Also, I was thinking the name of the main group could be optional.
Internally, it can be given the name 'default group' or 'main group' for
status reporting.
The above could also be written as:
'2(london: 2[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
backward compatible:
In JSON, while validating we may have to check if it starts with '{' to go
Something worth noticing, application_name can begin with {.
for JSON parsing else proceed with the current method.
A,B,C = 1[A,B,C]. This can be added in the new parser code.
This makes sense. We could do the same for JSON-based format as well
by reusing the in-memory structure used to deparse the blob when the
former grammar is used as well.
If I validate s_s_name JSON syntax, I will definitely use JSONB,
rather than JSON.
Because JSONB has some useful operation functions for adding node,
deleting node to s_s_name today.
But the down side of using JSONB for s_s_name is that it could switch
in key name order place.(and remove duplicate key)
For example in the syntax Michael suggested,
* JSON (just casting JSON)
json
{ +
sync_standby_names: +
{ +
quorum:2, +
nodes: +
[ +
{priority:1,group:cluster1},+
{quorum:2,nodes:[node1,node2,node3]}+
] +
},+
groups: +
{ +
cluster1:[node11,node12,node13], +
cluster2:[node21,node22,node23] +
} +
}
* JSONB (using jsonb_pretty)
jsonb_pretty
Re: [HACKERS] deparsing utility commands
On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera alvhe...@2ndquadrant.com
wrote:
David Steele wrote:
I have reviewed and tested this patch and everything looks good to me.
It also looks like you added better coverage for schema DDL, which is a
welcome addition.
Thanks -- I have pushed this now.
Hi,
I've tried compiling the 0003-ddl_deparse-extension part from
http://www.postgresql.org/message-id/20150409161419.gc4...@alvh.no-ip.org
on current master and that has failed because the 0002 part hasn't been
actually pushed (I've asked Alvaro off the list about this, that's how I
know the reason ;-).
I was able to update the 0002 part so it applies cleanly (updated version
attached), and then the contrib module compiles after one minor change and
seems to work.
I've started to look into what it would take to move 0002's code to the
extension itself, and I've got a question about use of printTypmod() in
format_type_detailed():
if (typemod = 0)
*typemodstr = printTypmod(NULL, typemod, typeform-typmodout);
else
*typemodstr = pstrdup();
Given that printTypmod() does psprintf(%s%s) one way or the other,
shouldn't we pass an empty string here instead of NULL as typname argument?
My hope is to get this test module extended quite a bit, not only to
cover existing commands, but also so that it causes future changes to
cause failure unless command collection is considered. (In a previous
version of this patch, there was a test mode that ran everything in the
serial_schedule of regular regression tests. That was IMV a good way to
ensure that new commands were also tested to run under command
collection. That hasn't been enabled in the new test module, and I
think it's necessary.)
If anyone wants to contribute to the test module so that more is
covered, that would be much appreciated.
I'm planning to have a look at this part also.
--
Alex
From 5381f5efafd1c2fd29b7c842e5ef1edd552d545e Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin oleksandr.shul...@zalando.de
Date: Wed, 29 Jul 2015 11:09:56 +0200
Subject: [PATCH] ddl_deparse core support
---
src/backend/commands/seclabel.c | 5 +
src/backend/commands/sequence.c | 34 +++
src/backend/commands/tablecmds.c| 3 +-
src/backend/utils/adt/format_type.c | 127 +
src/backend/utils/adt/regproc.c | 101 +--
src/backend/utils/adt/ruleutils.c | 516
src/include/commands/sequence.h | 1 +
src/include/utils/builtins.h| 4 +
src/include/utils/ruleutils.h | 21 +-
9 files changed, 730 insertions(+), 82 deletions(-)
diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 1ef98ce..aa4de97 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -63,6 +63,11 @@ ExecSecLabelStmt(SecLabelStmt *stmt)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(must specify provider when multiple security label providers have been loaded)));
provider = (LabelProvider *) linitial(label_provider_list);
+ /*
+ * Set the provider in the statement so that DDL deparse can use
+ * provider explicitly in generated statement.
+ */
+ stmt-provider = (char *) provider-provider_name;
}
else
{
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 9c1037f..b0f8003 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1517,6 +1517,40 @@ process_owned_by(Relation seqrel, List *owned_by)
relation_close(tablerel, NoLock);
}
+/*
+ * Return sequence parameters, detailed
+ */
+Form_pg_sequence
+get_sequence_values(Oid sequenceId)
+{
+ Buffer buf;
+ SeqTable elm;
+ Relation seqrel;
+ HeapTupleData seqtuple;
+ Form_pg_sequence seq;
+ Form_pg_sequence retSeq;
+
+ retSeq = palloc(sizeof(FormData_pg_sequence));
+
+ /* open and AccessShareLock sequence */
+ init_sequence(sequenceId, elm, seqrel);
+
+ if (pg_class_aclcheck(sequenceId, GetUserId(),
+ ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg(permission denied for sequence %s,
+ RelationGetRelationName(seqrel;
+
+ seq = read_seq_tuple(elm, seqrel, buf, seqtuple);
+
+ memcpy(retSeq, seq, sizeof(FormData_pg_sequence));
+
+ UnlockReleaseBuffer(buf);
+ relation_close(seqrel, NoLock);
+
+ return retSeq;
+}
/*
* Return sequence parameters, for use by information schema
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d394713..5058f9f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8169,7 +8169,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
if (!list_member_oid(tab-changedConstraintOids,
foundObject.objectId))
{
- char *defstring = pg_get_constraintdef_string(foundObject.objectId);
+ char *defstring = pg_get_constraintdef_string(foundObject.objectId,
+
Re: [HACKERS] REVOKE [ADMIN OPTION FOR] ROLE
On Tue, Jul 28, 2015 at 10:51 AM, Egor Rogov e.ro...@postgrespro.ru wrote: Well, I looked into a draft of SQL:2003. It basically says that cascade for revoke role statement must behave the same way as for revoke privilege statement. That is, from standard's point of view we have a code issue. Still I doubt about usefulness of this behavior. Do we really need it in PostgreSQL? I think it's useful, as long as there are use-cases where instead of granting privileges on the specific classes of database objects (i.e. SELECT on all tables in a given schema) a role is granted instead which 'accumulates' those privileges. This is the case in our environment, and, while we are not using ADMIN OPTION, I can imagine it might be used in order to 'delegate' assignment of privileges from the central authority to responsible sub-authorities in different departments. Then, if you need to revoke those (i.e. because the structure of departments had changed), it's enough to REVOKE ..FROM.. CASCADE instead of getting through each individual assignment case. Kind regards, -- Oleksii -- 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] MultiXact member wraparound protections are now enabled
On 28 July 2015 at 14:20, Robert Haas robertmh...@gmail.com wrote: On Sat, Jul 25, 2015 at 4:11 AM, Simon Riggs si...@2ndquadrant.com wrote: On 22 July 2015 at 21:45, Robert Haas robertmh...@gmail.com wrote: But it seemed to me that this could be rather confusing. I thought it would be better to be explicit about whether the protections are enabled in all cases. That way, (1) if you see the message saying they are enabled, they are enabled; (2) if you see the message saying they are disabled, they are disabled; and (3) if you see neither message, your version does not have those protections. (3) would imply that we can't ever remove the message, in case people think they are unprotected. If we display (1) and then we find a further bug, where does that leave us? Do we put a second really, really fixed message? AIUI this refers to a bug fix, its not like we've invented some anti-virus mode to actively prevent or even scan for further error. I'm not sure why we need a message to say a bug fix has been applied; that is what the release notes are for. If something is disabled, we should say so, but otherwise silence means safety and success. Well, I think that we can eventually downgrade or remove the message once (1) we've actually fixed all of the known multixact bugs and (2) a couple of years have gone by and most people are in the clear. But right now, we've still got significant bugs unfixed. https://wiki.postgresql.org/wiki/MultiXact_Bugs Therefore, in my opinion, anything that might make it harder to debug problems with the MultiXact system is premature at this point. The detective work that it took to figure out the chain of events that led to the problem fixed in 068cfadf9e2190bdd50a30d19efc7c9f0b825b5e was difficult; I wanted to make sure that future debugging would be easier, not harder. I still think that's the right decision, but I recognize that not everyone agrees. I do now, thanks for explaining. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] [PATCH] Generalized JSON output functions
On July 13 I wrote: Yes, but I think the plugin is the right place to do it. What is more, this won't actually prevent you completely from producing non-ECMAScript compliant JSON, since json or jsonb values containing offending numerics won't be caught, AIUI. But a fairly simple to write function that reparsed and fixed the JSON inside the decoder would work. The OP admitted that this was a serious flaw in his approach. In fact, given that a json value can contain an offending numeric value, any approach which doesn't involve reparsing is pretty much bound to fail. I agree that was a critical omission in my thinking. Now, back to the whitespace issue: I could submit a patch to unify the whitespace w/o all the hairy callbacks. Did we have the consensus here: no spaces whatsoever unless some *_pretty function is used? -- Alex
Re: [HACKERS] pg_basebackup and replication slots
On 2015-07-29 09:17:04 +0100, Simon Riggs wrote: On 29 July 2015 at 09:09, Andres Freund and...@anarazel.de wrote: The point of using a temporary slot is to not have a leftover slot afterwards, reserving resources. Especially important if the basebackup actually failed... Creating a slot and then deleting it if the session disconnects does not successfully provide the functionality desired above. Uh? If you create the slot, start streaming, and then start the basebackup, it does. It does *not* guarantee that the base backup can be converted into a replica, but it's sufficient to guarantee it can brought out of recovery. -- 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] LWLock deadlock and gdb advice
Hi, Finally getting to this. On 2015-07-15 18:44:03 +0300, Heikki Linnakangas wrote: Previously, LWLockAcquireWithVar set the variable associated with the lock atomically with acquiring it. Before the lwlock-scalability changes, that was straightforward because you held the spinlock anyway, but it's a lot harder/expensive now. So I changed the way acquiring a lock with a variable works. There is now a separate flag, LW_FLAG_VAR_SET, which indicates that the current lock holder has updated the variable. The LWLockAcquireWithVar function is gone - you now just use LWLockAcquire(), which always clears the LW_FLAG_VAR_SET flag, and you can call LWLockUpdateVar() after that if you want to set the variable immediately. This passes make check, but I haven't done any testing beyond that. Does this look sane to you? The prime thing I dislike about the patch is how long it now holds the spinlock in WaitForVar. I don't understand why that's necessary? There's no need to hold a spinlock until after the mustwait = (pg_atomic_read_u32(lock-state) LW_VAL_EXCLUSIVE) != 0; unless I miss something? In an earlier email you say: After the spinlock is released above, but before the LWLockQueueSelf() call, it's possible that another backend comes in, acquires the lock, changes the variable's value, and releases the lock again. In 9.4, the spinlock was not released until the process was queued. But that's not a problem. The updater in that case will have queued a wakeup for all waiters, including WaitForVar()? I'll try to reproduce the problem now. But I do wonder if it's possibly just the missing spinlock during the update. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 2015-07-29 12:47:01 +0100, Simon Riggs wrote: On 29 July 2015 at 11:43, Andres Freund and...@anarazel.de wrote: On 2015-07-29 09:17:04 +0100, Simon Riggs wrote: On 29 July 2015 at 09:09, Andres Freund and...@anarazel.de wrote: The point of using a temporary slot is to not have a leftover slot afterwards, reserving resources. Especially important if the basebackup actually failed... Creating a slot and then deleting it if the session disconnects does not successfully provide the functionality desired above. Uh? If you create the slot, start streaming, and then start the basebackup, it does. It does *not* guarantee that the base backup can be converted into a replica, but it's sufficient to guarantee it can brought out of recovery. Perhaps we are misunderstanding the word it here. it can be brought out of recovery? The finished base backup. You appear to be saying that a backup that disconnects before completion is useful in some way. How so? I'm not trying to say that at all. As far as I understand this subthread the goal is to have a pg_basebackup that internally creates a slot, so it can guarantee that all the required WAL is present till streamed out by -X stream/fetch. The problem with just creating a slot is that it'd leak if pg_basebackup is killed, or the connection breaks. The idea with the ephemeral slot is that it'd automatically kept alive until the base backup is complete, but would also automatically be dropped if the base backup fails. Makes sense? We pretty much have all the required infrastructure for that in slot.c, it's just not exposed to the replication protocol. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LWLock deadlock and gdb advice
On 2015-07-29 15:14:23 +0300, Heikki Linnakangas wrote: Ah, ok, that should work, as long as you also re-check the variable's value after queueing. Want to write the patch, or should I? I'll try. Shouldn't be too hard. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 2015-07-29 13:45:22 +0100, Simon Riggs wrote: So this would be needed when creating a standalone backup that would not be persistently connected to the master, yet we want to bring it up as a live/writable server in a single command I'm not understanding what you mean with 'single command' here. Currently there's no way to do this safely without configuring a high enough wal_keep_segments. You can (since yesterday) manually create a slot, run pg_basebackup, and drop the slot. But that's not safe if your script is interrupted somehow. Since many base backups are run in a non-interactive fashion asking for intervention to clean up in that case imo is not an acceptable answer. and we want to make it easy to script in case our script is killed? Or the connection dies/is killed, or the server is restarted, or ... -- 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] Don'st start streaming after creating a slot in pg_receivexlog
On 29 July 2015 at 12:51, Michael Paquier michael.paqu...@gmail.com wrote: In short, I would propose the following: - Have --create-slot only create a slot, then exit for both pg_recvlogical and pg_receivexlog. - Have --drop-slot drop a slot, then exit. It makes more sense to create one new utility to issue replication commands than to enhance multiple utility commands to have bizarre looking additional features and modes. pg_reputil --create-slot pg_reputil --drop-slot etc though I prefer the idea of using psql and the already existing slot functions than doing all of this extra code that needs maintaining. - Remove the --start switch in pg_recvlogical, and let the utility start streaming by default. By doing so both utilities will behave similarly with the same set of options. +1 That way all the utilities have just one thing they do. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] LWLock deadlock and gdb advice
On 2015-07-29 14:22:23 +0200, Andres Freund wrote:
On 2015-07-29 15:14:23 +0300, Heikki Linnakangas wrote:
Ah, ok, that should work, as long as you also re-check the variable's value
after queueing. Want to write the patch, or should I?
I'll try. Shouldn't be too hard.
What do you think about something roughly like the attached?
- Andres
From 2879408d8da7714ab6af6238dfe1c8a535800af3 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 29 Jul 2015 15:06:54 +0200
Subject: [PATCH] Other way to fix the issues around the broken LWLock variable
support.
---
src/backend/storage/lmgr/lwlock.c | 105 +-
1 file changed, 69 insertions(+), 36 deletions(-)
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index e5566d1..c64f20d 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1066,7 +1066,15 @@ LWLockAcquireCommon(LWLock *lock, LWLockMode mode, uint64 *valptr, uint64 val)
/* If there's a variable associated with this lock, initialize it */
if (valptr)
+ {
+#ifdef LWLOCK_STATS
+ lwstats-spin_delay_count += SpinLockAcquire(lock-mutex);
+#else
+ SpinLockAcquire(lock-mutex);
+#endif
*valptr = val;
+ SpinLockRelease(lock-mutex);
+ }
TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), T_ID(lock), mode);
@@ -1259,6 +1267,60 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
}
/*
+ * Does the lwlock in its current state need to wait for the variable value to
+ * change?
+ *
+ * If we don't need to wait, and it's because the value of the variable has
+ * changed, store the current value in newval.
+ *
+ * *result is set to true if the lock was free, and false otherwise.
+ */
+static bool
+LWLockConflictsWithVar(LWLock *lock,
+ uint64 *valptr, uint64 oldval, uint64 *newval,
+ bool *result)
+{
+ bool mustwait;
+ uint64 value;
+
+ mustwait = (pg_atomic_read_u32(lock-state) LW_VAL_EXCLUSIVE) != 0;
+
+ if (!mustwait)
+ {
+ *result = true;
+ return false;
+ }
+
+ /*
+ * Perform comparison using spinlock as we can't rely on atomic 64 bit
+ * reads/stores. On platforms with a way to do atomic 64 bit reads/writes
+ * the spinlock can be optimized away.
+ */
+#ifdef LWLOCK_STATS
+ lwstats-spin_delay_count += SpinLockAcquire(lock-mutex);
+#else
+ SpinLockAcquire(lock-mutex);
+#endif
+
+ *result = false;
+
+ value = *valptr;
+ if (value != oldval)
+ {
+ mustwait = false;
+ *newval = value;
+ }
+ else
+ {
+ mustwait = true;
+ }
+
+ SpinLockRelease(lock-mutex);
+
+ return mustwait;
+}
+
+/*
* LWLockWaitForVar - Wait until lock is free, or a variable is updated.
*
* If the lock is held and *valptr equals oldval, waits until the lock is
@@ -1313,39 +1375,9 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
for (;;)
{
bool mustwait;
- uint64 value;
-
- mustwait = (pg_atomic_read_u32(lock-state) LW_VAL_EXCLUSIVE) != 0;
-
- if (mustwait)
- {
- /*
- * Perform comparison using spinlock as we can't rely on atomic 64
- * bit reads/stores.
- */
-#ifdef LWLOCK_STATS
- lwstats-spin_delay_count += SpinLockAcquire(lock-mutex);
-#else
- SpinLockAcquire(lock-mutex);
-#endif
- /*
- * XXX: We can significantly optimize this on platforms with 64bit
- * atomics.
- */
- value = *valptr;
- if (value != oldval)
- {
-result = false;
-mustwait = false;
-*newval = value;
- }
- else
-mustwait = true;
- SpinLockRelease(lock-mutex);
- }
- else
- mustwait = false;
+ mustwait = LWLockConflictsWithVar(lock, valptr, oldval, newval,
+ result);
if (!mustwait)
break;/* the lock was free or value didn't match */
@@ -1365,12 +1397,13 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
pg_atomic_fetch_or_u32(lock-state, LW_FLAG_RELEASE_OK);
/*
- * We're now guaranteed to be woken up if necessary. Recheck the
- * lock's state.
+ * We're now guaranteed to be woken up if necessary. Recheck the lock
+ * and variables state.
*/
- mustwait = (pg_atomic_read_u32(lock-state) LW_VAL_EXCLUSIVE) != 0;
+ mustwait = LWLockConflictsWithVar(lock, valptr, oldval, newval,
+ result);
- /* Ok, lock is free after we queued ourselves. Undo queueing. */
+ /* Ok, no conflict after we queued ourselves. Undo queueing. */
if (!mustwait)
{
LOG_LWDEBUG(LWLockWaitForVar, lock, free, undoing queue);
--
2.4.0.rc2.1.g3d6bc9a
--
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] Don'st start streaming after creating a slot in pg_receivexlog
On 2015-07-29 13:53:31 +0100, Simon Riggs wrote: It makes more sense to create one new utility to issue replication commands than to enhance multiple utility commands to have bizarre looking additional features and modes. pg_reputil --create-slot pg_reputil --drop-slot etc Logical slots behave differently than physical slots and need different options. So to me there's a fair point in having support for manipulating logical slot in a tool around logical slots. Additionally that support was there in 9.4 and I know at least of three scripts that call pg_recvlogical to create logical slots. Admittedly one of them is of my own creation. though I prefer the idea of using psql and the already existing slot functions than doing all of this extra code that needs maintaining. It's not that uncommon to have replicas only access the primary for replication type connections. So it seems completely sensible to use the replication protocol to manage slots. And that you can't really do with psql. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for N synchronous standby servers - take 2
On Wed, Jul 29, 2015 at 9:03 PM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Tue, Jul 21, 2015 at 3:50 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Mon, Jul 20, 2015 at 9:59 PM, Beena Emerson memissemer...@gmail.com
wrote:
Simon Riggs wrote:
The choice between formats is not
solely predicated on whether we have multi-line support.
I still think writing down some actual use cases would help bring the
discussion to a conclusion. Inventing a general facility is hard without
some clear goals about what we need to support.
We need to at least support the following:
- Grouping: Specify of standbys along with the minimum number of commits
required from the group.
- Group Type: Groups can either be priority or quorum group.
As far as I understood at the lowest level a group is just an alias
for a list of nodes, quorum or priority are properties that can be
applied to a group of nodes when this group is used in the expression
to define what means synchronous commit.
- Group names: to simplify status reporting
- Nesting: At least 2 levels of nesting
If I am following correctly, at the first level there is the
definition of the top level objects, like groups and sync expression.
The grouping and using same application_name different server is similar.
How does the same application_name different server work?
In the same of a priority group both nodes get the same priority,
imagine for example that we need to wait for 2 nodes with lower
priority: node1 with priority 1, node2 with priority 2 and again node2
with priority 2, we would wait for the first one, and then one of the
second. In quorum group, any of them could be qualified for selection.
Using JSON, sync rep parameter to replicate in 2 different clusters could be
written as:
{remotes:
{quorum: 2,
servers: [{london:
{priority: 2,
servers: [lndn1, lndn2, lndn3]
}}
,
{nyc:
{priority: 1,
servers: [ny1, ny2]
}}
]
}
}
The same parameter in the new language (as suggested above) could be written
as:
'remotes: 2(london: 1[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
OK, there is a typo. That's actually 2(london: 2[lndn1, lndn2, lndn3],
nyc: 1[ny1, ny2]) in your grammar. Honestly, if we want group aliases,
I think that JSON makes the most sense. One of the advantage of a
group is that you can use it in several places in the blob and set
different properties into it, hence we should be able to define a
group out of the sync expression.
Hence I would think that something like that makes more sense:
{
sync_standby_names:
{
quorum:2,
nodes:
[
{priority:1,group:cluster1},
{quorum:2,nodes:[node1,node2,node3]}
]
},
groups:
{
cluster1:[node11,node12,node13],
cluster2:[node21,node22,node23]
}
}
Also, I was thinking the name of the main group could be optional.
Internally, it can be given the name 'default group' or 'main group' for
status reporting.
The above could also be written as:
'2(london: 2[lndn1, lndn2, lndn3], nyc: 1[ny1, ny2])'
backward compatible:
In JSON, while validating we may have to check if it starts with '{' to go
Something worth noticing, application_name can begin with {.
for JSON parsing else proceed with the current method.
A,B,C = 1[A,B,C]. This can be added in the new parser code.
This makes sense. We could do the same for JSON-based format as well
by reusing the in-memory structure used to deparse the blob when the
former grammar is used as well.
If I validate s_s_name JSON syntax, I will definitely use JSONB,
rather than JSON.
Because JSONB has some useful operation functions for adding node,
deleting node to s_s_name today.
But the down side of using JSONB for s_s_name is that it could switch
in key name order place.(and remove duplicate key)
For example in the syntax Michael suggested,
[...]
group and sync_standby_names has been switched place. I'm not sure
it's good for the users.
I think that's perfectly fine.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup and replication slots
On 29 July 2015 at 13:00, Andres Freund and...@anarazel.de wrote: As far as I understand this subthread the goal is to have a pg_basebackup that internally creates a slot, so it can guarantee that all the required WAL is present till streamed out by -X stream/fetch. The problem with just creating a slot is that it'd leak if pg_basebackup is killed, or the connection breaks. The idea with the ephemeral slot is that it'd automatically kept alive until the base backup is complete, but would also automatically be dropped if the base backup fails. Makes sense? So this would be needed when creating a standalone backup that would not be persistently connected to the master, yet we want to bring it up as a live/writable server in a single command, and we want to make it easy to script in case our script is killed? -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On Wed, Jul 29, 2015 at 10:15 PM, Andres Freund wrote: It's not that uncommon to have replicas only access the primary for replication type connections. So it seems completely sensible to use the replication protocol to manage slots. And that you can't really do with psql. Actually, you can. CREATE_REPLICATION_SLOT is one of the commands that work with psql, but that's not really practical compared to what pg_recvlogical and pg_receivexlog can do. $ psql -d replication=1 =# CREATE_REPLICATION_SLOT hoge PHYSICAL; slot_name | consistent_point | snapshot_name | output_plugin ---+--+---+--- hoge | 0/0 | null | null (1 row) -- 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] Don'st start streaming after creating a slot in pg_receivexlog
On 2015-07-29 22:17:27 +0900, Michael Paquier wrote: Here is a patch implementing those things. IMO if-not-exists does not make much sense anymore What? It's rather useful to be able to discern between 'slot was already there' and 'oops, some error occured'. -1 To me the pg_recvlogical changes are pretty pointless. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LWLock deadlock and gdb advice
On 2015-07-29 14:55:54 +0300, Heikki Linnakangas wrote: On 07/29/2015 02:39 PM, Andres Freund wrote: In an earlier email you say: After the spinlock is released above, but before the LWLockQueueSelf() call, it's possible that another backend comes in, acquires the lock, changes the variable's value, and releases the lock again. In 9.4, the spinlock was not released until the process was queued. But that's not a problem. The updater in that case will have queued a wakeup for all waiters, including WaitForVar()? If you release the spinlock before LWLockQueueSelf(), then no. It's possible that the backend you wanted to wait for updates the variable's value before you've queued up. Or even releases the lock, and it gets re-acquired by another backend, before you've queued up. For normal locks the equivalent problem is solved by re-checking wether a conflicting lock is still held after enqueuing. Why don't we do the same here? Doing it that way has the big advantage that we can just remove the spinlocks entirely on platforms with atomic 64bit reads/writes. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Freeze avoidance of very large table.
On Thu, Jul 16, 2015 at 8:51 PM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, Jul 15, 2015 at 3:07 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Wed, Jul 15, 2015 at 12:55 AM, Simon Riggs si...@2ndquadrant.com wrote:
On 10 July 2015 at 15:11, Sawada Masahiko sawada.m...@gmail.com wrote:
Oops, I had forgotten to add new file heapfuncs.c.
Latest patch is attached.
I think we've established the approach is desirable and defined the way
forwards for this, so this is looking good.
If we want to move stuff like pg_stattuple, pg_freespacemap into core,
we could move them into heapfuncs.c.
Some of my requests haven't been actioned yet, so I personally would not
commit this yet. I am happy to continue as reviewer/committer unless others
wish to take over.
The main missing item is pg_upgrade support, which won't happen by end of
CF1, so I am marking this as Returned With Feedback. Hopefully we can review
this again before CF2.
I appreciate your reviewing.
Yeah, the pg_upgrade support and regression test for VFM patch is
almost done now, I will submit the patch in this week after testing it
.
Attached patch is latest v9 patch.
I added:
- regression test for visibility map (visibilitymap.sql and
visibilitymap.out files)
- pg_upgrade support (rewriting vm file to vfm file)
- regression test for pg_upgrade
Previous patch has some fail to apply, so attached the rebased patch.
Catalog version is not decided yet, so we will need to rewrite
VISIBILITY_MAP_FROZEN_BIT_CAT_VER in pg_upgrade.h
Please review it.
Regards,
--
Masahiko Sawada
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index 22c5f7a..b1b6a06 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -87,7 +87,7 @@ statapprox_heap(Relation rel, output_type *stat)
* If the page has only visible tuples, then we can find out the free
* space from the FSM and move on.
*/
- if (visibilitymap_test(rel, blkno, vmbuffer))
+ if (visibilitymap_test(rel, blkno, vmbuffer, VISIBILITYMAP_ALL_VISIBLE))
{
freespace = GetRecordedFreeSpace(rel, blkno);
stat-tuple_len += BLCKSZ - freespace;
diff --git a/src/backend/access/heap/Makefile b/src/backend/access/heap/Makefile
index b83d496..806ce27 100644
--- a/src/backend/access/heap/Makefile
+++ b/src/backend/access/heap/Makefile
@@ -12,6 +12,7 @@ subdir = src/backend/access/heap
top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
-OBJS = heapam.o hio.o pruneheap.o rewriteheap.o syncscan.o tuptoaster.o visibilitymap.o
+OBJS = heapam.o hio.o pruneheap.o rewriteheap.o syncscan.o tuptoaster.o visibilitymap.o \
+ heapfuncs.o
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 050efdc..2dbabc8 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2176,8 +2176,9 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
/*
- * Find buffer to insert this tuple into. If the page is all visible,
- * this will also pin the requisite visibility map page.
+ * Find buffer to insert this tuple into. If the page is all visible
+ * or all frozen, this will also pin the requisite visibility map and
+ * frozen map page.
*/
buffer = RelationGetBufferForTuple(relation, heaptup-t_len,
InvalidBuffer, options, bistate,
@@ -2192,7 +2193,11 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
if (PageIsAllVisible(BufferGetPage(buffer)))
{
all_visible_cleared = true;
+
+ /* all-frozen information is also cleared at the same time */
PageClearAllVisible(BufferGetPage(buffer));
+ PageClearAllFrozen(BufferGetPage(buffer));
+
visibilitymap_clear(relation,
ItemPointerGetBlockNumber((heaptup-t_self)),
vmbuffer);
@@ -2493,7 +2498,11 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
if (PageIsAllVisible(page))
{
all_visible_cleared = true;
+
+ /* all-frozen information is also cleared at the same time */
PageClearAllVisible(page);
+ PageClearAllFrozen(page);
+
visibilitymap_clear(relation,
BufferGetBlockNumber(buffer),
vmbuffer);
@@ -2776,9 +2785,9 @@ heap_delete(Relation relation, ItemPointer tid,
/*
* If we didn't pin the visibility map page and the page has become all
- * visible while we were busy locking the buffer, we'll have to unlock and
- * re-lock, to avoid holding the buffer lock across an I/O. That's a bit
- * unfortunate, but hopefully shouldn't happen often.
+ * visible or all frozen while we were busy locking the buffer, we'll
+ * have to unlock and re-lock, to avoid holding the buffer lock across an
+ * I/O. That's a bit unfortunate, but hopefully shouldn't happen often.
*/
if (vmbuffer == InvalidBuffer PageIsAllVisible(page))
{
@@ -2970,10
Re: [HACKERS] A little RLS oversight?
Robert, * Robert Haas (robertmh...@gmail.com) wrote: On Mon, Jul 27, 2015 at 4:58 PM, Stephen Frost sfr...@snowman.net wrote: I would expect that if the current user has permission to bypass RLS, and they have set row_security to OFF, then it should be off for all tables that they have access to, regardless of how they access those tables (directly or through a view). If it really is intentional that RLS remains active when querying through a view not owned by the table's owner, then the other calls to check_enable_rls() should probably be left as they were, since the table might have been updated through such a view and that code can't really tell at that point. Joe and I were discussing this earlier and it was certainly intentional that RLS still be enabled if you're querying through a view as the RLS rights of the view owner are used, not your own. Note that we don't allow a user to assume the BYPASSRLS right of the view owner though, also intentionally. That seems inconsistent. If querying through a view doesn't adopt the BYPASSRLS right (or lack thereof) of the view owner, and I agree that it shouldn't, then it also shouldn't disregard the fact that the person issuing the query has that right. That's not how other role attributes currently work though, specifically thinking of superuser. Even when running a query as a superuser you can get a permission denied if you're querying a view where the view owner hasn't got access to the relation under the view. =# create role r1; CREATE ROLE =*# create role r2; CREATE ROLE =*# create table t1 (c1 int); CREATE TABLE =*# alter table t1 owner to r1; ALTER TABLE =*# create view v1 as select * from t1; CREATE VIEW =*# alter view v1 owner to r2; ALTER VIEW =*# select * from v1; ERROR: permission denied for relation t1 In other words, we've made a decision, when going through views, to test ACLs based on who owns the view. We do that in all cases, not only sometimes. Now, when querying a view, whose BYPASSRLS privilege do we consult? It should either be the person issuing the query in all cases, or the view owner in all cases, not some hybrid of the two. For superuser (the only similar precedent that we have, I believe), we go based on the view owner, but that isn't quite the same as BYPASSRLS. The reason this doesn't hold is that you have to use a combination of BYPASSRLS and row_security=off to actually bypass RLS, unlike the superuser role attribute which is just always on if you've got it. If having BYPASSRLS simply always meant don't do any RLS then we could use the superuser precedent to use what the view owner has, but at least for my part, I'm a lot happier with BYPASSRLS and row_security than with superuser and would rather we continue in that direction, where the user has the choice of if they want their role attribute to be in effect or not. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Don'st start streaming after creating a slot in pg_receivexlog
On Wed, Jul 29, 2015 at 8:51 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Wed, Jul 29, 2015 at 5:02 PM, Heikki Linnakangas wrote: Hmm. pg_receivelogical is basically a debugging tool. I don't think anyone will have it integrated into production scripts etc. So maybe we could just change it. This sounds good to me as well. I'm not sure I understand the proposal though. In short, I would propose the following: - Have --create-slot only create a slot, then exit for both pg_recvlogical and pg_receivexlog. - Have --drop-slot drop a slot, then exit. - Remove the --start switch in pg_recvlogical, and let the utility start streaming by default. By doing so both utilities will behave similarly with the same set of options. If you don't specify --create-slot, nor --start, what would the program do? Nothing? Complain if neither --create-slot nor --drop-slot nor --start are specified: $ pg_recvlogical -f - --slot toto -d postgres pg_recvlogical: at least one action needs to be specified Try pg_recvlogical --help for more information. $ echo $? 1 Here is a patch implementing those things. IMO if-not-exists does not make much sense anymore. Documentation has been shuffled a bit as well. -- Michael 20150729_pgrecv_slots.patch Description: binary/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel Seq Scan
On Wed, Jul 29, 2015 at 7:32 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: Hi Amit, Could you tell me the code intention around ExecInitFunnel()? ExecInitFunnel() calls InitFunnel() that opens the relation to be scanned by the underlying PartialSeqScan and setup ss_ScanTupleSlot of its scanstate. The main need is for relation descriptor which is then required to set the scan tuple's slot. Basically it is required for tuples flowing from worker which will use the scan tuple slot of FunnelState. According to the comment of InitFunnel(), it open the relation and takes appropriate lock on it. However, an equivalent initialization is also done on InitPartialScanRelation(). Why does it acquire the relation lock twice? I think locking twice is not required, it is just that I have used the API ExecOpenScanRelation() which is used during other node's initialisation due to which it lock's twice. I think in general it should be harmless. Thanks, I could get reason of the implementation. It looks to me this design is not problematic even if Funnel gets capability to have multiple sub-plans thus is not associated with a particular relation as long as target-list and projection-info are appropriately initialized. Best regards, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.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] Support for N synchronous standby servers - take 2
Hello,
Just looking at how the 2 differnt methods can be used to set the s_s_names
value.
1. For a simple case where quorum is required for a single group the JSON
could be:
{
sync_standby_names:
{
quorum:2,
nodes:
[ node1,node2,node3 ]
}
}
or
{
sync_standby_names:
{
quorum:2,
group: cluster1
},
groups:
{
cluster1:[node1,node2,node3]
}
}
Language:
2(node1, node2, node3)
2. For having quorum between different groups and node:
{
sync_standby_names:
{
quorum:2,
nodes:
[
{priority:1,nodes:[node0]},
{quorum:2,group: cluster1}
]
},
groups:
{
cluster1:[node1,node2,node3]
}
}
or
{
sync_standby_names:
{
quorum:2,
nodes:
[
{priority:1,group: cluster2},
{quorum:2,group: cluster1}
]
},
groups:
{
cluster1:[node1,node2,node3],
cluster2:[node0]
}
}
Language:
2 (node0, cluster1: 2(node1, node2, node3))
Since there will not be many nesting and grouping, I still prefer new
language to JSON.
I understand one can easily, modify/add groups in JSON using in built
functions but I think changes will not be done too often.
-
Beena Emerson
--
View this message in context:
http://postgresql.nabble.com/Support-for-N-synchronous-standby-servers-take-2-tp5849384p5860197.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
