Re: [HACKERS] [Patch] Log SSL certificate verification errors
On Sat, Nov 11, 2017 at 3:34 AM, Graham Leggettwrote: > Currently neither the server side nor the client side SSL certificate verify > callback does anything, leading to potential hair-tearing-out moments. > > The following patch to master implements logging of all certificate > verification failures, as well as (crucially) which certificates failed to > verify, and at what depth, so the admin can zoom in straight onto the problem > without any guessing. Could you attach as a file to this thread a patch that can be easily applied? Using git --format-patch or simply diff is just fine. Here are also some community guidelines on the matter: https://wiki.postgresql.org/wiki/Submitting_a_Patch And if you are looking for feedback, you should register it to the next commit fest: https://commitfest.postgresql.org/16/ -- 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_upgrade to clusters with a different WAL segment size
On Sat, Nov 11, 2017 at 12:46 AM, Bossart, Nathanwrote: > Allowing changes to the WAL segment size during pg_upgrade seems like a > nice way to avoid needing a dump and load, so I would like to propose > adding support for this. I'd be happy to submit patches for this in the > next commitfest. That's a worthy goal. -- 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] Simplify ACL handling for large objects and removal of superuser() checks
On Sat, Nov 11, 2017 at 12:50 AM, Robert Haaswrote: > On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane wrote: >> I think as far as that goes, we can just change to "Therefore, by default >> their use is restricted ...". Then I suggest adding a para >> after that, with wording along the lines of >> >> It is possible to GRANT use of server-side lo_import and lo_export to >> non-superusers, but careful consideration of the security implications >> is required. A malicious user of such privileges could easily parlay >> them into becoming superuser (for example by rewriting server >> configuration files), or could attack the rest of the server's file >> system without bothering to obtain database superuser privileges as >> such. Access to roles having such privilege must therefore be guarded >> just as carefully as access to superuser roles. Nonetheless, if use >> of server-side lo_import or lo_export is needed for some routine task, >> it's safer to use a role of this sort than full superuser privilege, >> as that helps to reduce the risk of damage from accidental errors. > > +1. That seems like great language to me. +1. Not convinced that mentioning wrappers is worth the complication. Experienced admins likely already know such matters. -- 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] Simplify ACL handling for large objects and removal of superuser() checks
Michael, Tom, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Fri, Nov 10, 2017 at 10:00 AM, Tom Lanewrote: > > Stephen Frost writes: > >> I'm guessing no, which essentially means that *we* consider access to > >> lo_import/lo_export to be equivilant to superuser and therefore we're > >> not going to implement anything to try and prevent the user who has > >> access to those functions from becoming superuser. If we aren't willing > >> to do that, then how can we really say that there's some difference > >> between access to these functions and being a superuser? > > > > We seem to be talking past each other. Yes, if a user has malicious > > intentions, it's possibly to parlay lo_export into obtaining a superuser > > login (I'm less sure that that's necessarily true for lo_import). > > That does NOT make it "equivalent", except perhaps in the view of someone > > who is only considering blocking malevolent actors. It does not mean that > > there's no value in preventing a task that needs to run lo_export from > > being able to accidentally destroy any data in the database. There's a > > range of situations where you are concerned about accidents and errors, > > not malicious intent; but your argument ignores those use-cases. > > That will not sound much as a surprise as I spawned the original > thread, but like Robert I understand that getting rid of all superuser > checks is a goal that we are trying to reach to allow admins to have > more flexibility in handling permissions to a subset of objects. > Forcing an admin to give full superuser rights to one user willing to > work only on LOs import and export is a wrong concept. The problem I have with this is that 'full superuser rights' actually are being granted by this. You're able to read any file and write any file on the filesystem that the PG OS user can. How is that not 'full superuser rights'? The concerns about a mistake being made are just as serious as they are with someone who has superuser rights as someone could pretty easily end up overwriting the control file or various other extremely important bits of the system. This isn't just about what 'blackhats' can do with this. As I mentioned up-thread, if we want to make it so that non-superusers can use lo_import/lo_export, then we should do that in a way that allows the administrator to specify exactly the rights they really want to give, based on a use-case for them. There's no use-case for allowing someone to use lo_export() to overwrite pg_control. The use-case would be to allow them to export to a specific directory (or perhaps a set of directories), or to read from same. The same is true of COPY. Further, I wonder what would happen if we allow this and then someone comes along and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed (ideally with things cleaned up and tightened up to avoid there being issues using it) to address the actual use-case for these functions to be available to a non-superuser. We wouldn't be able to change these functions to start checking the 'directory' rights or we would break existing non-superuser usage of them. I imagine we could create additional functions which check the 'directory' privileges, but that's hardly ideal, imv. I'm disappointed to apparently be in the minority on this, but that seems to be the case unless someone else wishes to comment. While I appreciate the discussion, I'm certainly no more convinced today than I was when I first saw this go in that allowing these functions to be granted to non-superusers does anything but effectively make them into superusers who aren't explicitly identified as superusers. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: Improve bitmap costing for lossy pages
On Thu, Nov 9, 2017 at 3:55 AM, amul sulwrote: > It took me a little while to understand this calculation. You have moved this > code from tbm_create(), but I think you should move the following > comment as well: I made an adjustment that I hope will address your concern here, made a few other adjustments, and committed this. One point of concern that wasn't entirely addressed in the above discussion is the accuracy of this formula: + lossy_pages = Max(0, heap_pages - maxentries / 2); When I first looked at Dilip's test results, I thought maybe this formula was way off. But on closer study, the formula does a decent (not fantastic) job of estimating the number of exact pages. The fact that the number of lossy pages is off is just because the Mackert and Lohman formula is overestimating how many pages are fetched. Now, in Dilip's results, this formula more often than not - but not invariably - predicted more exact pages than we actually got. So possibly instead of maxentries / 2 we could subtract maxentries or some other multiple of maxentries. I don't know what's actually best here, but I think there's a strong argument that this is an improvement as it stands, and we can adjust it later if it becomes clear what would be better. -- 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] LDAP URI decoding bugs
On Sat, Nov 11, 2017 at 8:37 AM, Peter Eisentrautwrote: > On 11/6/17 23:30, Michael Paquier wrote: >> On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro >> wrote: >>> 1. If you set up a pg_hba.conf with a URL that lacks a base DN or >>> hostname, hba.c will segfault on startup when it tries to pstrdup a >>> null pointer. Examples: ldapurl="ldap://localhost; and >>> ldapurl="ldap://;. >>> >>> 2. If we fail to bind but have no binddn configured, we'll pass NULL >>> to ereport (snprint?) for %s, which segfaults on some libc >>> implementations. That crash requires more effort to reproduce but you >>> can see pretty clearly a few lines above in auth.c that it can be >>> NULL. (I'm surprised Coverity didn't complain about that. Maybe it >>> can't see this code due to macros.) > > committed and backpatched Thanks! I suppose someone might eventually want to go further and teach it to understand such bare URLs or missing options (ie leaving out any bits you want and falling back to the ldap library's defaults, which come from places like env variables, .ldaprc and /etc/ldap.conf, the way that "ldapsearch" and other tools manage to work with reasonable defaults, or at least only need to be set up in one place for all your LDAP-client software). I'm not planning to work on that. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallelize queries containing initplans
Robert Haaswrites: > On Fri, Nov 10, 2017 at 2:24 PM, Tom Lane wrote: >> Also, I wonder whether the InvalidOid hack in SS_assign_special_param >> requires commentary. It might be safer to use a valid type OID there, >> perhaps VOIDOID or INTERNALOID. > There is existing precedent for using InvalidOid to denote the absence > of a parameter -- cf. copyParamList, SerializeParamList. OK, fair enough. I was concerned that this was adding a corner case not otherwise seen with Params, but evidently not. 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] Planning counters in pg_stat_statements
On Tue, Nov 7, 2017 at 6:39 PM, Tsunakawa, Takayukiwrote: > From: pgsql-hackers-ow...@postgresql.org >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Thomas Munro >> I have often wanted $SUBJECT and was happy to find that Fujii-san had posted >> a patch five years ago[1]. The reception then seemed positive. >> So here is a refurbished and (hopefully) improved version of his patch with >> a new column for the replan count. Thoughts? > > That's a timely proposal. I sometimes faced performance problems where the > time pg_stat_statements shows is much shorter than the application perceives. > The latest experience was that the execution time of a transaction, which > consists of dozens of DMLs and COMMIT, was about 200ms from the application's > perspective, while pg_stat_statements showed only about 10ms in total. The > network should not be the cause because the application ran on the same host > as the database server. I wanted to know how long the parsing and planning > time was. Note that this patch doesn't include the parse or parse analysis times. I guess they would be less interesting? But perhaps someone would want to have the complete query production line measured. BTW the reason I was looking into this was because an Oracle user asked me how to see "hard parse" times on Postgres, and I've talked to others who seem strangely concerned with "parsing" time. On Oracle I believe that term covers (among other things) actually planning, and I guess planning is the most interesting component. Planning is the thing I've wanted to measure myself, to diagnose problems relating to partition/inheritance planning and join explosions and to figure out which things should be changed to PREPARE/EXECUTE. Perhaps a separate parse/analysis counter might become more interesting for us if we ever add automatic plan cache so you could assess how often you're getting an implicit prepared statement (something like Oracle's "soft parse")? > BTW, the current pg_stat_statement shows unexpected time for COMMIT. I > expect it to include the whole COMMIT processing, including the long WAL > flush and sync rep wait. However, it only shows the time for the transaction > state change in memory. That's an interesting point. You could install a transaction hook to measure that easily enough, but I'm not sure how useful it'd be: you'd be grouping together COMMIT timing data from transactions that are doing very different things (including nothing). Would that tell you anything actionable? If you include commit time for COMMIT statements then you'd also have to decide whether to include it for DML statements that run in an implicit transaction. The trouble with that is that the same statement inside an explicit transaction wouldn't have any commit time, so you'd be mixing oranges and apples. I guess you could fix that by putting adding "commits" and "commit_time" columns (= counters for this statement run as implicit transaction), but I wonder if commit time monitoring really belongs somewhere else. For sync rep waits, that's what the pg_stat_replication.XXX_lag columns tell you. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallelize queries containing initplans
On Fri, Nov 10, 2017 at 2:24 PM, Tom Lanewrote: > Robert Haas writes: >> I decided to try instead teaching the planner to keep track of the >> types of PARAM_EXEC parameters as they were created, and that seems to >> work fine. See 0001, attached. > > I did not look at the other part, but 0001 looks reasonable to me. Thanks for looking. > I might quibble with the grammar in the generate_new_param comment: > > - * need to record the PARAM_EXEC slot number as being allocated. > + * need to make sure we have record the type in paramExecTypes (otherwise, > + * there won't be a slot allocated for it). > */ > > I'd just go with "need to record the type in ..." Noted. > Also, I wonder whether the InvalidOid hack in SS_assign_special_param > requires commentary. It might be safer to use a valid type OID there, > perhaps VOIDOID or INTERNALOID. I think it's pretty straightforward -- if, as the existing comments say, no Param node will be present and no value will be stored, then we do not and cannot record the type of the value that we're not storing. There is existing precedent for using InvalidOid to denote the absence of a parameter -- cf. copyParamList, SerializeParamList. That convention was not invented by me or the parallel query stuff, although it has become more widespread for that reason. I am disinclined to have this patch invent a New Way To Do 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] LDAP URI decoding bugs
On 11/6/17 23:30, Michael Paquier wrote: > On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro >wrote: >> 1. If you set up a pg_hba.conf with a URL that lacks a base DN or >> hostname, hba.c will segfault on startup when it tries to pstrdup a >> null pointer. Examples: ldapurl="ldap://localhost; and >> ldapurl="ldap://;. >> >> 2. If we fail to bind but have no binddn configured, we'll pass NULL >> to ereport (snprint?) for %s, which segfaults on some libc >> implementations. That crash requires more effort to reproduce but you >> can see pretty clearly a few lines above in auth.c that it can be >> NULL. (I'm surprised Coverity didn't complain about that. Maybe it >> can't see this code due to macros.) committed and backpatched -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key
On Wed, Sep 27, 2017 at 7:07 AM, amul sulwrote: > Attaching POC patch that throws an error in the case of a concurrent update > to an already deleted tuple due to UPDATE of partition key[1]. > > In a normal update new tuple is linked to the old one via ctid forming > a chain of tuple versions but UPDATE of partition key[1] move tuple > from one partition to an another partition table which breaks that chain. This patch needs a rebase. It has one whitespace-only hunk that should possibly be excluded. I think the basic idea of this is sound. Either you or Amit need to document the behavior in the user-facing documentation, and it needs tests that hit every single one of the new error checks you've added (obviously, the tests will only work in combination with Amit's patch). The isolation should be sufficient to write such tests. It needs some more extensive comments as well. The fact that we're assigning a meaning to ip_blkid -> InvalidBlockNumber is a big deal, and should at least be documented in itemptr.h in the comments for the ItemPointerData structure. I suspect ExecOnConflictUpdate needs an update for the HeapTupleUpdated case similar to what you've done elsewhere. -- 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] parallelize queries containing initplans
Robert Haaswrites: > I decided to try instead teaching the planner to keep track of the > types of PARAM_EXEC parameters as they were created, and that seems to > work fine. See 0001, attached. I did not look at the other part, but 0001 looks reasonable to me. I might quibble with the grammar in the generate_new_param comment: - * need to record the PARAM_EXEC slot number as being allocated. + * need to make sure we have record the type in paramExecTypes (otherwise, + * there won't be a slot allocated for it). */ I'd just go with "need to record the type in ..." Also, I wonder whether the InvalidOid hack in SS_assign_special_param requires commentary. It might be safer to use a valid type OID there, perhaps VOIDOID or INTERNALOID. 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] Add some const decorations to prototypes
On 11/10/17 10:29, Fabien COELHO wrote: > Or basically all is fine, I'm just nitpicking for nothing, shame on me. > > As I said, I rather like more precise declarations. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallelize queries containing initplans
On Tue, Nov 7, 2017 at 4:45 AM, Amit Kapilawrote: > As mentioned, changed the status of the patch in CF app. I spent some time reviewing this patch today and found myself still quite uncomfortable with the fact that it was adding execution-time work to track the types of parameters - types that would usually not even be used. I found the changes to nodeNestLoop.c to be particularly objectionable, because we could end up doing the work over and over when it is actually not needed at all, or at most once. I decided to try instead teaching the planner to keep track of the types of PARAM_EXEC parameters as they were created, and that seems to work fine. See 0001, attached. 0002, attached, is my worked-over version of the rest of the patch. I moved the code that serializes and deserializes PARAM_EXEC from nodeSubplan.c -- which seemed like a strange choice - to execParallel.c. I removed the type OID from the serialization format because there's no reason to do that any more; the worker already knows the types from the plan. I did some renaming of the functions involved and some adjustment of the comments to refer to "PARAM_EXEC parameters" instead of initPlan parameters, because there's no reason that I can see why this can only work for initPlans. A Gather node on the inner side of a nested loop doesn't sound like a great idea, but I think this infrastructure could handle it (though it would need some more planner work). I broke a lot of long lines in your version of the patch into multiple lines; please try to be attentive to this issue when writing patches in general, as it is a bit tedious to go through and insert line breaks in many places. Please let me know your thoughts on the attached patches. Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company 0001-param-exec-types-v1.patch Description: Binary data 0002-pq-pushdown-initplan-rebased.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [Patch] Log SSL certificate verification errors
Hi all, Currently neither the server side nor the client side SSL certificate verify callback does anything, leading to potential hair-tearing-out moments. The following patch to master implements logging of all certificate verification failures, as well as (crucially) which certificates failed to verify, and at what depth, so the admin can zoom in straight onto the problem without any guessing. The SSL test suite runs without regression: Little-Net:ssl minfrin$ make check rm -rf '/Users/minfrin/src/postgres/postgres-trunk'/tmp_install /bin/sh ../../../config/install-sh -c -d '/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log /Applications/Xcode.app/Contents/Developer/usr/bin/make -C '../../..' DESTDIR='/Users/minfrin/src/postgres/postgres-trunk'/tmp_install install >'/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log/install.log 2>&1 rm -rf '/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'/tmp_check /bin/sh ../../../config/install-sh -c -d '/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'/tmp_check cd . && TESTDIR='/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl' PATH="/Users/minfrin/src/postgres/postgres-trunk/tmp_install/tmp/postgresql/bin:$PATH" DYLD_LIBRARY_PATH="/Users/minfrin/src/postgres/postgres-trunk/tmp_install/tmp/postgresql/lib" PGPORT='65432' PG_REGRESS='/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl/../../../src/test/regress/pg_regress' /opt/local/bin/prove -I ../../../src/test/perl/ -I . t/*.pl t/001_ssltests.pl .. ok All tests successful. Files=1, Tests=40, 6 wallclock secs ( 0.04 usr 0.01 sys + 2.01 cusr 1.21 csys = 3.27 CPU) Result: PASS Index: src/backend/libpq/be-secure-openssl.c === --- src/backend/libpq/be-secure-openssl.c (revision 61109) +++ src/backend/libpq/be-secure-openssl.c (working copy) @@ -999,9 +999,9 @@ /* * Certificate verification callback * - * This callback allows us to log intermediate problems during - * verification, but for now we'll see if the final error message - * contains enough information. + * There are 50 ways to leave your lover, and 67 ways to fail + * certificate verification. Log details of all failed certificate + * verification results. * * This callback also allows us to override the default acceptance * criteria (e.g., accepting self-signed or expired certs), but @@ -1010,6 +1010,28 @@ static int verify_cb(int ok, X509_STORE_CTX *ctx) { + char *subject, *issuer; + X509 *cert; + int err, depth; + + if (!ok) + { + cert = X509_STORE_CTX_get_current_cert(ctx); + err = X509_STORE_CTX_get_error(ctx); + depth = X509_STORE_CTX_get_error_depth(ctx); + + subject = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0); + issuer = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0); + + ereport(COMMERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("SSL certificate verification result: %s (depth %d, subject '%s', issuer '%s')", + X509_verify_cert_error_string(err), depth, subject, issuer))); + + OPENSSL_free(subject); + OPENSSL_free(issuer); + } + return ok; } Index: src/interfaces/libpq/fe-secure-openssl.c === --- src/interfaces/libpq/fe-secure-openssl.c(revision 61109) +++ src/interfaces/libpq/fe-secure-openssl.c(working copy) @@ -82,6 +82,8 @@ static bool ssl_lib_initialized = false; +static int ssl_ex_data_index; + #ifdef ENABLE_THREAD_SAFETY static long ssl_open_connections = 0; @@ -182,6 +184,7 @@ */ SOCK_ERRNO_SET(0); ERR_clear_error(); + resetPQExpBuffer(>errorMessage); n = SSL_read(conn->ssl, ptr, len); err = SSL_get_error(conn->ssl, n); @@ -200,7 +203,7 @@ if (n < 0) { /* Not supposed to happen, so we don't translate the msg */ - printfPQExpBuffer(>errorMessage, + appendPQExpBuffer(>errorMessage, "SSL_read failed but did not provide error information\n"); /* assume the connection is broken */ result_errno = ECONNRESET; @@ -224,13 +227,13 @@ result_errno = SOCK_ERRNO; if (result_errno == EPIPE || result_errno == ECONNRESET) - printfPQExpBuffer(>errorMessage, + appendPQExpBuffer(>errorMessage,
Re: [HACKERS] Add some const decorations to prototypes
On 11/10/17 11:42, Fabien COELHO wrote: > After your explanation, and on third thoughts, ISTM that the assignment > should not include "const" in the explicit cast, i.e., use > >extern void * msg_func(void); >const char * msg = (char *) msg_func(); > > The variable or field is constant, not what the function returns, so > >const char * msg = (const char *) msg_func(); > > does not really make full sense to me, and moreover the compiler does not > complain without the const. The compiler knows how to handle the char * -> const char * case, but not the char ** -> const char ** case. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix bloom WAL tap test
I wrote: > Is there anything we can do to cut the runtime of the TAP test to > the point where running it by default wouldn't be so painful? As an experiment, I tried simply cutting the size of the test table 10X: diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl index 1b319c9..566abf9 100644 --- a/contrib/bloom/t/001_wal.pl +++ b/contrib/bloom/t/001_wal.pl @@ -57,7 +57,7 @@ $node_standby->start; $node_master->safe_psql("postgres", "CREATE EXTENSION bloom;"); $node_master->safe_psql("postgres", "CREATE TABLE tst (i int4, t text);"); $node_master->safe_psql("postgres", -"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,10) i;" +"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,1) i;" ); $node_master->safe_psql("postgres", "CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);"); @@ -72,7 +72,7 @@ for my $i (1 .. 10) test_index_replay("delete $i"); $node_master->safe_psql("postgres", "VACUUM tst;"); test_index_replay("vacuum $i"); - my ($start, $end) = (11 + ($i - 1) * 1, 10 + $i * 1); + my ($start, $end) = (10001 + ($i - 1) * 1000, 1 + $i * 1000); $node_master->safe_psql("postgres", "INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series($start,$end) i;" ); This about halved the runtime of the TAP test, and it changed the coverage footprint not at all according to lcov. (Said coverage is only marginally better than what we get without running the bloom TAP test, AFAICT.) It seems like some effort could be put into both shortening this test and improving the amount of code it exercises. 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] Fix bloom WAL tap test
Michael Paquierwrites: > On Thu, Nov 9, 2017 at 7:51 PM, Alexander Korotkov > wrote: >> OK, then so be it :) > Thanks for the new version. This one, as well as the switch to > psql_safe in > https://www.postgresql.org/message-id/CAPpHfduxgEYF_0BTs-mxGC4=w5sw8rnUbq9BSTp1Wq7=nwr...@mail.gmail.com > are good for a committer lookup IMO. The safe_psql change is a clear bug fix, so I've pushed it. However, as far as adding the TAP test to the standard test suite goes, I've got two complaints about this patch: 1. It doesn't (I think, can't test) do anything to run the test on Windows. 2. The TAP test is enormously slower than the standard test. On my development workstation, "make installcheck" in contrib/bloom takes about 0.5 sec; this patch increases that to 11.6 sec. I'm not too happy about that as a developer, and even less so as an owner of some fairly slow buildfarm critters. I'd say that this might be the reason the TAP test wasn't part of the standard tests to begin with. Is there anything we can do to cut the runtime of the TAP test to the point where running it by default wouldn't be so painful? 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] Add some const decorations to prototypes
Fabien COELHOwrites: >> LWLockTrancheArray = (char **) >> MemoryContextAllocZero(TopMemoryContext, >> LWLockTranchesAllocated * sizeof(char *)); > After your explanation, and on third thoughts, ISTM that the assignment > should not include "const" in the explicit cast, Can't get terribly excited about that one way or the other. I think the statement would be OK as-is, and it would also be fine as LWLockTrancheArray = (const char **) MemoryContextAllocZero(TopMemoryContext, LWLockTranchesAllocated * sizeof(const char *)); The other two possible combinations are not good of course --- not that they'd generate invalid code, but that they'd require readers to expend brain cells convincing themselves that the code wasn't wrong. > ... and moreover the compiler does not > complain without the const. Arguing on the basis of what your compiler does is a pretty shaky basis. It's not impossible that someone else's compiler would complain if the casted-to type isn't identical to the variable's type. I tend to agree that a compiler *should* allow "char **" to be cast to "const char **" silently, but that isn't necessarily what happens in the real world. 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] Add some const decorations to prototypes
Hello Tom, ISTM That there is still at least one strange cast: +static const char **LWLockTrancheArray = NULL; + LWLockTrancheArray = (const char **) // twice These are not cases of "cheating". This is just the return value of a memory allocation function being cast from void * to the appropriate result type. That is an orthogonal style decision that I have maintained in these cases. Would it make sense that the function returns "const void *", i.e. the cast is not on the const part but on the pointer type part? Certainly not -- if malloc-like functions returned "const void *" then you could never write on allocated space without having casted away const. Ok. Sure. LWLockTrancheArray = (char **) MemoryContextAllocZero(TopMemoryContext, LWLockTranchesAllocated * sizeof(char *)); and the reader can see by inspection that the calculation of how much to allocate (so many char-* items) is consistent with the char-** result. It is not necessary to go find the declaration of LWLockTrancheArray and verify that it's char **, because we trust the compiler to whine if char ** isn't assignment-compatible with that. But if we left off the cast and just assigned the function result directly to the variable, then there would be nothing directly tying the variable's type to this allocation computation. Thanks for the reflesher course about the intricacies of "const". After your explanation, and on third thoughts, ISTM that the assignment should not include "const" in the explicit cast, i.e., use extern void * msg_func(void); const char * msg = (char *) msg_func(); The variable or field is constant, not what the function returns, so const char * msg = (const char *) msg_func(); does not really make full sense to me, and moreover the compiler does not complain without the const. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect comment for build_child_join_rel
On Fri, Nov 10, 2017 at 4:34 AM, Etsuro Fujitawrote: > Here is a small patch for $Subject. Good catch. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: psql command \graw
2017-11-10 16:38 GMT+01:00 Fabien COELHO: > > Hello, > > Maybe I'm missing something, but it looks that it could be made to work >>> without adding another boolean. >>> >> >> The tuples only cannot be disabled, because then other parts print number >> of rows >> >> postgres=# \pset format unaligned >> Output format is unaligned. >> >> postgres=# select 10 as a, 20 as b; >> a|b >> 10|20 >> (1 row) < >> > > Argh. Too bad. > > I'm not at ease with having two bools which nearly mean the opposite one > of the other but not exactly... however I'm not sure that there is a > simpler way out of this, some exception handling is needed one way or the > other, either within the header or within the footer... Maybe the whole > topt logic should be reviewed, but that is not the point of this patch. > I don't think so it is not correct - this mean tuples only + header. Probably the best implementation is something three state - all, tuples only, tuples only and header. But it mean much more changes in psql logic - not adequate to size of this patch > So I switched the patch to "ready for committer". > Thank you very much Regards Pavel > > -- > Fabien. >
Re: [HACKERS] Aggregates push-down to partitions
On 10.11.2017 12:15, Ashutosh Bapat wrote: Maybe in this thread[1] your described problem are solved through introducing Parallel Append node? 1. https://www.postgresql.org/message-id/CAJ3gD9dy0K_E8r727heqXoBmWZ83HwLFwdcaSSmBQ1%2BS%2BvRuUQ%40mail.gmail.com You may want to review [2] and [3] as well. [2] https://www.postgresql.org/message-id/9666.1491295317@localhost [3] https://www.postgresql.org/message-id/CAM2+6=V64_xhstVHie0Rz=kpeqnljmzt_e314p0jat_oj9m...@mail.gmail.com Thank you very much for this references. I applied partition-wise-agg-v6 patches and for partitioned tables it works perfectly: shard=# explain select count(*) from orders; QUERY PLAN --- Finalize Aggregate (cost=100415.29..100415.30 rows=1 width=8) -> Append (cost=50207.63..100415.29 rows=2 width=8) -> Partial Aggregate (cost=50207.63..50207.64 rows=1 width=8) -> Foreign Scan on orders_0 (cost=101.00..50195.13 rows=5000 width=0) -> Partial Aggregate (cost=50207.63..50207.64 rows=1 width=8) -> Foreign Scan on orders_1 (cost=101.00..50195.13 rows=5000 width=0) (6 rows) But I wonder why the same optimization is not applied to normal inherited table: shard=# explain select count(*) from base; QUERY PLAN -- Aggregate (cost=44087.99..44088.00 rows=1 width=8) -> Append (cost=0.00..39079.46 rows=2003414 width=0) -> Seq Scan on base (cost=0.00..0.00 rows=1 width=0) -> Seq Scan on derived1 (cost=0.00..14425.00 rows=100 width=0) -> Seq Scan on derived2 (cost=0.00..14425.00 rows=100 width=0) -> Foreign Scan on derived_fdw (cost=100.00..212.39 rows=3413 width=0) (6 rows) Are there some principle problems? -- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
Fabien COELHOwrites: >>> ISTM That there is still at least one strange cast: +static const char **LWLockTrancheArray = NULL; + LWLockTrancheArray = (const char **) // twice >> These are not cases of "cheating". This is just the return value of a >> memory allocation function being cast from void * to the appropriate >> result type. That is an orthogonal style decision that I have >> maintained in these cases. > Would it make sense that the function returns "const void *", i.e. the > cast is not on the const part but on the pointer type part? Certainly not -- if malloc-like functions returned "const void *" then you could never write on allocated space without having casted away const. In any case, what's shown above is not involving any funny stuff, because the type of LWLockTrancheArray is pointer to non-const array of pointers to const char strings. So it's correct to be assigning a non-const pointer to it. If it were written like "const char * const *" then the issues would be quite different. As for your followup --- yes, we could just omit the cast altogether and the compiler wouldn't complain, but that is not better style IMO. The value of the cast really is to document what type we're expecting the expression to produce. In context, that statement (today) is LWLockTrancheArray = (char **) MemoryContextAllocZero(TopMemoryContext, LWLockTranchesAllocated * sizeof(char *)); and the reader can see by inspection that the calculation of how much to allocate (so many char-* items) is consistent with the char-** result. It is not necessary to go find the declaration of LWLockTrancheArray and verify that it's char **, because we trust the compiler to whine if char ** isn't assignment-compatible with that. But if we left off the cast and just assigned the function result directly to the variable, then there would be nothing directly tying the variable's type to this allocation computation. 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] Simplify ACL handling for large objects and removal of superuser() checks
On Fri, Nov 10, 2017 at 10:34 AM, Tom Lanewrote: > I think as far as that goes, we can just change to "Therefore, by default > their use is restricted ...". Then I suggest adding a para > after that, with wording along the lines of > > It is possible to GRANT use of server-side lo_import and lo_export to > non-superusers, but careful consideration of the security implications > is required. A malicious user of such privileges could easily parlay > them into becoming superuser (for example by rewriting server > configuration files), or could attack the rest of the server's file > system without bothering to obtain database superuser privileges as > such. Access to roles having such privilege must therefore be guarded > just as carefully as access to superuser roles. Nonetheless, if use > of server-side lo_import or lo_export is needed for some routine task, > it's safer to use a role of this sort than full superuser privilege, > as that helps to reduce the risk of damage from accidental errors. +1. That seems like great language to me. -- 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] [PATCH] A hook for session start
On Thu, Nov 9, 2017 at 9:08 PM, Michael Paquierwrote: > > On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello > wrote: > > On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier < michael.paqu...@gmail.com> > > wrote: > >> +++ b/src/test/modules/test_session_hooks/session_hooks.conf > >> @@ -0,0 +1 @@ > >> +shared_preload_libraries = 'test_session_hooks' > >> Don't you think that this should be a GUC? My previous comment > >> outlined that. I won't fight hard on that point in any case, don't > >> worry. I just want to make things clear :) > > > > Ooops... my fault... fixed! > > > > Thanks again!! > > +/* GUC variables */ > +static char *session_hook_username = NULL; > This causes the module to crash if test_session_hooks.username is not > set. I would recommend just assigning a default value, say "postgres". > Fixed. New version attached. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 05c5c19..d3156ad 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason; static MemoryContext row_description_context = NULL; static StringInfoData row_description_buf; +/* Hook for plugins to get control at start of session */ +session_start_hook_type session_start_hook = NULL; + /* * decls for routines only used in this file * @@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[], if (!IsUnderPostmaster) PgStartTime = GetCurrentTimestamp(); + if (session_start_hook) + (*session_start_hook) (); + /* * POSTGRES main processing loop begins here * diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 20f1d27..16ec376 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); +/* Hook for plugins to get control at end of session */ +session_end_hook_type session_end_hook = NULL; /*** InitPostgres support ***/ @@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg) * them explicitly. */ LockReleaseAll(USER_LOCKMETHOD, true); + + /* Hook at session end */ + if (session_end_hook) + (*session_end_hook) (); } diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index f8c535c..9f05bfb 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string; extern int max_stack_depth; extern int PostAuthDelay; +/* Hook for plugins to get control at start and end of session */ +typedef void (*session_start_hook_type) (void); +typedef void (*session_end_hook_type) (void); + +extern PGDLLIMPORT session_start_hook_type session_start_hook; +extern PGDLLIMPORT session_end_hook_type session_end_hook; + /* GUC-configurable parameters */ typedef enum diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index b7ed0af..a3c8c1e 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -11,6 +11,7 @@ SUBDIRS = \ snapshot_too_old \ test_ddl_deparse \ test_extensions \ + test_session_hooks \ test_parser \ test_pg_dump \ test_rbtree \ diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore new file mode 100644 index 000..5dcb3ff --- /dev/null +++ b/src/test/modules/test_session_hooks/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile new file mode 100644 index 000..c5c3860 --- /dev/null +++ b/src/test/modules/test_session_hooks/Makefile @@ -0,0 +1,21 @@ +# src/test/modules/test_session_hooks/Makefile + +MODULES = test_session_hooks +PGFILEDESC = "test_session_hooks - Test session hooks with an extension" + +EXTENSION = test_session_hooks +DATA = test_session_hooks--1.0.sql + +REGRESS = test_session_hooks +REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_session_hooks +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git
[HACKERS] pg_upgrade to clusters with a different WAL segment size
Hi hackers, The thread for the recent change to allow setting the WAL segment size at initdb time [0] included a bit of discussion regarding pg_upgrade [1], where it was suggested that relaxing an error check (presumably in check_control_data()) might be enough to upgrade servers to a different WAL segment size. We've had success with our initial testing of upgrades to larger WAL segment sizes, including post-upgrade pgbench runs. Beyond adjusting check_control_data(), it looks like the 'pg_resetwal -l' call in copy_xact_xlog_xid() may need to be adjusted to ensure that the WAL starting address is set to a valid value. Allowing changes to the WAL segment size during pg_upgrade seems like a nice way to avoid needing a dump and load, so I would like to propose adding support for this. I'd be happy to submit patches for this in the next commitfest. Nathan [0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fc49e24fa69a15efacd5b8958115ed9c43c48f9a [1] https://www.postgresql.org/message-id/20160826043926.pwkz45ksxpmfn4g6%40alap3.anarazel.de -- Sent 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: psql command \graw
Hello, Maybe I'm missing something, but it looks that it could be made to work without adding another boolean. The tuples only cannot be disabled, because then other parts print number of rows postgres=# \pset format unaligned Output format is unaligned. postgres=# select 10 as a, 20 as b; a|b 10|20 (1 row) < Argh. Too bad. I'm not at ease with having two bools which nearly mean the opposite one of the other but not exactly... however I'm not sure that there is a simpler way out of this, some exception handling is needed one way or the other, either within the header or within the footer... Maybe the whole topt logic should be reviewed, but that is not the point of this patch. So I switched the patch to "ready for committer". -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Michael Paquierwrites: > That will not sound much as a surprise as I spawned the original > thread, but like Robert I understand that getting rid of all superuser > checks is a goal that we are trying to reach to allow admins to have > more flexibility in handling permissions to a subset of objects. > Forcing an admin to give full superuser rights to one user willing to > work only on LOs import and export is a wrong concept. Right. I think the question may boil down to how we document this. The current para reads The server-side lo_import and lo_export functions behave considerably differently from their client-side analogs. These two functions read and write files in the server's file system, using the permissions of the database's owning user. Therefore, their use is restricted to superusers. In contrast, the client-side import and export functions read and write files in the client's file system, using the permissions of the client program. The client-side functions do not require superuser privilege. I think as far as that goes, we can just change to "Therefore, by default their use is restricted ...". Then I suggest adding a para after that, with wording along the lines of It is possible to GRANT use of server-side lo_import and lo_export to non-superusers, but careful consideration of the security implications is required. A malicious user of such privileges could easily parlay them into becoming superuser (for example by rewriting server configuration files), or could attack the rest of the server's file system without bothering to obtain database superuser privileges as such. Access to roles having such privilege must therefore be guarded just as carefully as access to superuser roles. Nonetheless, if use of server-side lo_import or lo_export is needed for some routine task, it's safer to use a role of this sort than full superuser privilege, as that helps to reduce the risk of damage from accidental errors. We could expand that by mentioning the possibility of wrapper functions, but it seems long enough already. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
Would it make sense that the function returns "const void *", i.e. the cast is not on the const part but on the pointer type part? Or maybe you do not really need a cast, the following code does not generate any warning when compiled with clang & gcc. #include // const void * would be ok as well void * msg_fun(void) { return "hello world"; } int main(void) { const char * msg = msg_fun(); printf("message: %s\n", msg); return 0; } Or basically all is fine, I'm just nitpicking for nothing, shame on me. As I said, I rather like more precise declarations. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
ISTM That there is still at least one strange cast: +static const char **LWLockTrancheArray = NULL; + LWLockTrancheArray = (const char **) // twice These are not cases of "cheating". This is just the return value of a memory allocation function being cast from void * to the appropriate result type. That is an orthogonal style decision that I have maintained in these cases. Ok. I'm at the limit of my C abilities. Your answer is about void * vs char *, I'm okay with that. My question was about no const / const in the same operation. Would it make sense that the function returns "const void *", i.e. the cast is not on the const part but on the pointer type part? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: multivariate histograms and MCV lists
> On Sep 12, 2017, at 2:06 PM, Tomas Vondra> wrote: > > Attached is an updated version of the patch, dealing with fallout of > 821fb8cdbf700a8aadbe12d5b46ca4e61be5a8a8 which touched the SGML > documentation for CREATE STATISTICS. Your patches need updating. Tom's commit 471d55859c11b40059aef7dd82f82b3a0dc338b1 changed src/bin/psql/describe.c, which breaks your 0001-multivariate-MCV-lists.patch.gz file. I reviewed the patch a few months ago, and as I recall, it looked good to me. I should review it again before approving it, though. mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] Faster processing at Gather node
On Fri, Nov 10, 2017 at 5:44 AM, Amit Kapilawrote: > I am seeing the assertion failure as below on executing the above > mentioned Create statement: > > TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File: > "heapam.c", Line: 2634) > server closed the connection unexpectedly > This probably means the server terminated abnormally OK, I see it now. Not sure why I couldn't reproduce this before. I think the problem is not actually with the code that I just wrote. What I'm seeing is that the slot descriptor's tdhasoid value is false for both the funnel slot and the result slot; therefore, we conclude that no projection is needed to remove the OIDs. That seems to make sense: if the funnel slot doesn't have OIDs and the result slot doesn't have OIDs either, then we don't need to remove them. Unfortunately, even though the funnel slot descriptor is marked tdhashoid = false, the tuples being stored there actually do have OIDs. And that is because they are coming from the underlying sequential scan, which *also* has OIDs despite the fact that tdhasoid for it's slot is false. This had me really confused until I realized that there are two processes involved. The problem is that we don't pass eflags down to the child process -- so in the user backend, everybody agrees that there shouldn't be OIDs anywhere, because EXEC_FLAG_WITHOUT_OIDS is set. In the parallel worker, however, it's not set, so the worker feels free to do whatever comes naturally, and in this test case that happens to be returning tuples with OIDs. Patch for this attached. I also noticed that the code that initializes the funnel slot is using its own PlanState rather than the outer plan's PlanState to call ExecContextForcesOids. I think that's formally incorrect, because the goal is to end up with a slot that is the same as the outer plan's slot. It doesn't matter because ExecContextForcesOids doesn't care which PlanState it gets passed, but the comments in ExecContextForcesOids imply that somebody it might, so perhaps it's best to clean that up. Patch for this attached, too. And here are the other patches again, too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company 0001-pass-eflags-to-worker-v1.patch Description: Binary data 0002-forces-oids-neatnikism-v1.patch Description: Binary data 0003-skip-gather-project-v2.patch Description: Binary data 0004-shm-mq-less-spinlocks-v2.patch Description: Binary data 0005-shm-mq-reduce-receiver-latch-set-v1.patch Description: Binary data 0006-remove-memory-leak-protection-v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Hi I am sending a review of last patch psql-server-version-1.patch.gz This patch is trivial - the most big problem is choosing correct name for GUC. I am thinking so server_version_raw is acceptable. I had to fix doc - see attached updated patch All tests passed. I'll mark this patch as ready for commiters Regards Pavel diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index d360fc4d58..924766fce7 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7956,8 +7956,22 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' -Reports the version number of the server as an integer. It is determined -by the value of PG_VERSION_NUM when building the server. +Reports the version number of the server as a short string. It is determined +by the value of PG_VERSION when building the server. + + + + + + server_version_raw (string) + + server_version_raw configuration parameter + + + + +Reports the version of the server as a long string. It is determined +by the value of PG_VERSION_STR when building the server. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index e520cdf3ba..50d6f0a8fc 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3770,11 +3770,14 @@ bar +SERVER_VERSION SERVER_VERSION_NAME SERVER_VERSION_NUM -The server's version number as a string, for +The server's version number as a long string, for +example PostgreSQL 11devel ..., +as a short string, for example 9.6.2, 10.1 or 11beta1, and in numeric form, for example 90602 or 11. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index c4c1afa084..49ff61246f 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -500,6 +500,7 @@ static char *locale_collate; static char *locale_ctype; static char *server_encoding_string; static char *server_version_string; +static char *server_version_raw_string; static int server_version_num; static char *timezone_string; static char *log_timezone_string; @@ -3295,6 +3296,18 @@ static struct config_string ConfigureNamesString[] = NULL, NULL, NULL }, + { + /* Can't be set in postgresql.conf */ + {"server_version_raw", PGC_INTERNAL, PRESET_OPTIONS, + gettext_noop("Shows the server version string."), + NULL, + GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE + }, + _version_raw_string, + PG_VERSION_STR, + NULL, NULL, NULL + }, + { /* Not for general use --- used by SET ROLE */ {"role", PGC_USERSET, UNGROUPED, diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 8cc4de3878..cfac89c8da 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -3210,7 +3210,8 @@ void SyncVariables(void) { char vbuf[32]; - const char *server_version; + const char *server_version, + *server_version_raw; /* get stuff from connection */ pset.encoding = PQclientEncoding(pset.db); @@ -3237,6 +3238,17 @@ SyncVariables(void) snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion); SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf); + server_version_raw = PQparameterStatus(pset.db, "server_version_raw"); + /* fall back again */ + if (!server_version_raw) + { + snprintf(vbuf, sizeof(vbuf), "PostgreSQL "); + formatPGVersionNumber(pset.sversion, true, vbuf + strlen(vbuf), + sizeof(vbuf) - strlen(vbuf)); + server_version_raw = vbuf; + } + SetVariable(pset.vars, "SERVER_VERSION", server_version_raw); + /* send stuff to it, too */ PQsetErrorVerbosity(pset.db, pset.verbosity); PQsetErrorContextVisibility(pset.db, pset.show_context); @@ -3255,6 +3267,7 @@ UnsyncVariables(void) SetVariable(pset.vars, "HOST", NULL); SetVariable(pset.vars, "PORT", NULL); SetVariable(pset.vars, "ENCODING", NULL); + SetVariable(pset.vars, "SERVER_VERSION", NULL); SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL); SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL); } diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c index 5335a91440..0418779f79 100644 --- a/src/interfaces/libpq/fe-protocol2.c +++ b/src/interfaces/libpq/fe-protocol2.c @@ -280,6 +280,10 @@ pqSetenvPoll(PGconn *conn) { char *ptr; + /* keep returned value */ + pqSaveParameterStatus(conn, "server_version_raw", + val); + /* strip off PostgreSQL part */ val += 11; diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 43ac5f5f11..eabb990d4e 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -767,3 +767,14 @@ NOTICE: text search configuration "no_such_config" does not exist select
Re: [HACKERS] Add some const decorations to prototypes
On 11/4/17 16:50, Fabien COELHO wrote: >>> Just leave it as char*. If you change the endptr argument you're going to >>> force every call site to change their return variable, and some of them >>> would end up having to cast away the const on their end. >> >> OK, here is an updated patch with the controversial bits removed. > > I'm in general favor in helping compilers, but if you have to cheat. > > ISTM That there is still at least one strange cast: > > +static const char **LWLockTrancheArray = NULL; > + LWLockTrancheArray = (const char **) // twice These are not cases of "cheating". This is just the return value of a memory allocation function being cast from void * to the appropriate result type. That is an orthogonal style decision that I have maintained in these cases. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transform for pl/perl
Hi 2017-10-24 14:27 GMT+02:00 Anthony Bykov: > There are some moments I should mention: > 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while > ["1","2"]::jsonb is transformed into AV ["1", "2"] > > 2. If there is a numeric value appear in jsonb, it will be transformed > to SVnv through string (Numeric->String->SV->SVnv). Not the best > solution, but as far as I understand this is usual practise in > postgresql to serialize Numerics and de-serialize them. > > 3. SVnv is transformed into jsonb through string > (SVnv->String->Numeric). > > An example may also be helpful to understand extension. So, as an > example, function "test" transforms incoming jsonb into perl, > transforms it back into jsonb and returns it. > > create extension jsonb_plperl cascade; > > create or replace function test(val jsonb) > returns jsonb > transform for type jsonb > language plperl > as $$ > return $_[0]; > $$; > > select test('{"1":1,"example": null}'::jsonb); > > I am looking to this patch: 1. the patch contains some artefacts - look the word "hstore" 2. I got lot of warnings make[1]: Vstupuje se do adresáře „/home/pavel/src/postgresql/contrib/jsonb_plperl“ gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3 -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c jsonb_plperl.c: In function ‘SV_FromJsonbValue’: jsonb_plperl.c:83:9: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] return (result); ^ jsonb_plperl.c: In function ‘SV_FromJsonb’: jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in this function [-Wmaybe-uninitialized] HV *object; ^~ In file included from /usr/lib64/perl5/CORE/perl.h:5644:0, from ../../src/pl/plperl/plperl.h:52, from jsonb_plperl.c:17: /usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized] #define newRV(a) Perl_newRV(aTHX_ a) ^~ jsonb_plperl.c:101:10: note: ‘value’ was declared here SV *value; ^ gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3 -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so jsonb_plperl.o -L../../src/port -L../../src/common -Wl,--as-needed -Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags -Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -fstack-protector-strong -L/usr/local/lib -L/usr/lib64/perl5/CORE -lperl -lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc make[1]: Opouští se adresář „/home/pavel/src/postgresql/contrib/jsonb_plperl“ [pavel@nemesis contrib]$ gcc --version gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2) Copyright (C) 2017 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. 3. regress tests passed 4. There are not any documentation - probably it should be part of PLPerl 5. The regress tests doesn't coverage other datatypes than numbers. I miss boolean, binary, object, ... Maybe using data::dumper or some similar can be interesting Note - it is great extension, I am pleasured so transformations are used. Regards Pavel > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] pg_basebackup --progress output for batch execution
Hi, Thanks for having a look at this patch. 2017-11-09 20:55 GMT-03:00 Jeff Janes: > On Fri, Sep 29, 2017 at 4:00 PM, Martin Marques > wrote: >> >> Hi, >> >> Some time ago I had to work on a system where I was cloning a standby >> using pg_basebackup, that didn't have screen or tmux. For that reason I >> redirected the output to a file and ran it with nohup. >> >> I normally (always actually ;) ) run pg_basebackup with --progress and >> --verbose so I can follow how much has been done. When done on a tty you >> get a nice progress bar with the percentage that has been cloned. >> >> The problem came with the execution and redirection of the output, as >> the --progress option will write a *very* long line! >> >> Back then I thought of writing a patch (actually someone suggested I do >> so) to add a --batch-mode option which would change the behavior >> pg_basebackup has when printing the output messages. > > > > While separate lines in the output file is better than one very long line, > it still doesn't seem so useful. If you aren't watching it in real time, > then you really need to have a timestamp on each line so that you can > interpret the output. The lines are about one second apart, but I don't > know robust that timing is under all conditions. I kind of disagree with your view here. If the cloning process takes many hours to complete (in my case, it was around 12 hours IIRC) you might want to peak at the log every now and then with tail. I do agree on adding a timestamp prefix to each line, as it's not clear from the code how often progress_report() is called. > I think I agree with Arthur that I'd rather have the decision made by > inspecting whether output is going to a tty, rather than by adding another > command line option. But maybe that is not detected robustly enough across > all platforms and circumstances? In this case, Arthur's idea is good, but would make the patch less generic/flexible for the end user. That's why I tried to reproduce what top does when executed with -b (Batch mode operation). There, it's the end user who decides how the output is formatted (well, saying it decides on formatting a bit of an overstatement, but you get it ;) ) An example where using isatty() might fail is if you run pg_basebackup from a tty but redirect the output to a file, I believe that in that case isatty() will return true, but it's very likely that the user might want batch mode output. But maybe we should also add Arthurs idea anyway (when not in batch mode), as it seems pretty lame to output progress in one line if you are not in a tty. Thoughts? -- Martín Marquéshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw
(2017/11/01 11:16), Robert Haas wrote: On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapatwrote: The view with WCO is local but the modification which violates WCO is being made on remote server by a trigger on remote table. Trying to control that doesn't seem to be a good idea, just like we can't control what rows get inserted on the foreign server when they violate local constraints. I think that's a fair point. For local constraints on foreign tables, it's the user's responsibility to ensure that those constraints matches the remote side, so we don't need to ensure those constraints locally. But I'm not sure if the same thing applies to WCOs on views defined on foreign tables, because in some case it's not possible to impose constraints on the remote side that match those WCOs, as I explained before. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Runtime Partition Pruning
On Thu, Nov 9, 2017 at 4:48 PM, Beena Emersonwrote: > Hello all, > > Here is the updated patch which is rebased over v10 of Amit Langote's > path towards faster pruning patch [1]. It modifies the PartScanKeyInfo > struct to hold expressions which is then evaluated by the executor to > fetch the correct partitions using the function. > Hi Beena, I have started looking into your patch, here few initial comments for your 0001 patch: 1. 351 + * Evaluate and store the ooutput of ExecInitExpr for each of the keys. Typo: ooutput 2. 822 + if (IsA(constexpr, Const) &_runtime) 823 + continue; 824 + 825 + if (IsA(constexpr, Param) &&!is_runtime) 826 + continue; 827 + Add space after '&&' 3. 1095 +* Generally for appendrel we don't fetch the clause from the the Typo: Double 'the' 4. 272 -/*- 273 + /*- Unnecessary hunk. 5. 313 + Node *n = eval_const_expressions_from_list(estate->es_param_list_info, val); 314 + Crossing 80 column window. Same at line # 323 & 325 6. 315 + keys->eqkeys_datums[i++] = ((Const *) n)->constvalue; Don’t we need a check for IsA(n, Const) or assert ? 7. 1011 + if (prmList) 1012 + context.boundParams = prmList; /* bound Params */ 1013 + else 1014 + context.boundParams = NULL; No need of prmList null check, context.boundParams = prmList; is enough. 8. It would be nice if you create a separate patch where you are moving PartScanKeyInfo and exporting function declaration. 9. Could you please add few regression tests, that would help in review & testing. 10. Could you please rebase your patch against latest "path toward faster partition pruning" patch by Amit. Regards, Amul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] Faster processing at Gather node
On Fri, Nov 10, 2017 at 9:48 AM, Amit Kapilawrote: > On Fri, Nov 10, 2017 at 8:36 AM, Robert Haas wrote: >> On Thu, Nov 9, 2017 at 9:31 PM, Amit Kapila wrote: >>> Have you set force_parallel_mode=regress; before running the >>> statement? >> >> Yes, I tried that first. >> >>> If so, then why you need to tune other parallel query >>> related parameters? >> >> Because I couldn't get it to break the other way, I then tried this. >> >> Instead of asking me what I did, can you tell me what I need to do? >> Maybe a self-contained reproducible test case including exactly what >> goes wrong on your end? >> > > I think we are missing something very basic because you should see the > failure by executing that statement in force_parallel_mode=regress > even in a freshly created database. > I am seeing the assertion failure as below on executing the above mentioned Create statement: TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File: "heapam.c", Line: 2634) server closed the connection unexpectedly This probably means the server terminated abnormally -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] No mention of CREATE STATISTICS in event trigger docs
A patch to fix this is attached. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services event_trigger_statistics_doc.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Incorrect comment for build_child_join_rel
Hi, Here is a small patch for $Subject. Best regards, Etsuro Fujita *** a/src/backend/optimizer/util/relnode.c --- b/src/backend/optimizer/util/relnode.c *** *** 676,683 build_join_rel(PlannerInfo *root, * 'sjinfo': child-join context info * 'restrictlist': list of RestrictInfo nodes that apply to this particular *pair of joinable relations ! * 'join_appinfos': list of AppendRelInfo nodes for base child relations ! *involved in this join */ RelOptInfo * build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, --- 676,682 * 'sjinfo': child-join context info * 'restrictlist': list of RestrictInfo nodes that apply to this particular *pair of joinable relations ! * 'jointype' is the join type (inner, left, full, etc) */ RelOptInfo * build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, -- Sent 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 --progress output for batch execution
On Thu, Nov 09, 2017 at 03:55:36PM -0800, Jeff Janes wrote: > > I think I agree with Arthur that I'd rather have the decision made by > inspecting whether output is going to a tty, rather than by adding another > command line option. But maybe that is not detected robustly enough across > all platforms and circumstances? > isatty() is used within Postgres code already (for example, pg_upgrade/util.c). However, it seems that on Windows isatty() is deprecated and it is recommended to use _isatty(). Moreover, on Windows it can give false positive result [1], if I'm not mistaken: > The _isatty function determines whether fd is associated with a character > device (a terminal, console, printer, or serial port). 1 - https://msdn.microsoft.com/en-us/library/f4s0ddew(v=vs.140).aspx -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Aggregates push-down to partitions
On Fri, Nov 10, 2017 at 12:20 AM, Maksim Milyutinwrote: > Hi Konstantin! >> I wonder if somebody already investigate this problem or working in this >> direction. >> May be there are already some patches proposed? >> I have searched hackers archive, but didn't find something relevant... >> Are there any suggestions about the best approach to implement this >> feature? >> > > Maybe in this thread[1] your described problem are solved through > introducing Parallel Append node? > > 1. > https://www.postgresql.org/message-id/CAJ3gD9dy0K_E8r727heqXoBmWZ83HwLFwdcaSSmBQ1%2BS%2BvRuUQ%40mail.gmail.com You may want to review [2] and [3] as well. [2] https://www.postgresql.org/message-id/9666.1491295317@localhost [3] https://www.postgresql.org/message-id/CAM2+6=V64_xhstVHie0Rz=kpeqnljmzt_e314p0jat_oj9m...@mail.gmail.com -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers