Re: [HACKERS] transition table behavior with inheritance appears broken
On Fri, Jun 23, 2017 at 02:39:48PM +0100, Andrew Gierth wrote: > > "Noah" == Noah Misch writes: > > Noah> This PostgreSQL 10 open item is past due for your status update. > Noah> Kindly send a status update within 24 hours, > > oops, sorry! I forgot to include a date in the last one, and in fact a > personal matter delayed things anyway. I expect to have this wrapped up > by 23:59 BST on the 24th. This PostgreSQL 10 open item is again past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Race conditions with WAL sender PID lookups
Noah Misch wrote: > IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due > for your status update. Please reacquaint yourself with the policy on open > item ownership[1] and then reply immediately. If I do not hear from you by > 2017-06-25 06:00 UTC, I will transfer this item to release management team > ownership without further notice. I volunteer to own this item. Next update before Wednesday 28th 19:00 CLT. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REPLICA IDENTITY FULL
> Thanks for the feedback. I have committed it after removing the > datumIsEqual() call. Thanks for the patch! I confirmed my examples now work. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] shift_sjis_2004 related autority files are remaining
> I don't have a clear opinion on this particular issue, but I think we > should have clarity on why particular files or code exist. So why do > these files exist and the others don't? Is it just the license? I think so. Many of those files are from http://ftp.unicode.org. There's no license description there, so I think we should not copy those files for safety reason. (I vaguely recall that they explicitly prohibited to distribute the files before but I could no find such a statement at this moment). gb-18030-2000.xml and windows-949-2000.xml are from https://ssl.icu-project.org/. I do not know what licenses those files use (maybe Apache). Regarding euc-jis-2004-std.txt and sjis-0213-2004-std.txt are from http://x0213.org. The license are described in the files. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] subscription worker signalling wal writer too much
Hi, On 2017-06-15 15:06:43 -0700, Jeff Janes wrote: > > Well, wal_writer_delay doesn't work if walwriter is in sleep mode, and > > this afaics would allow wal writer to go into sleep mode with half a > > page filled, and it'd not be woken up again until the page is filled. > > No? > > > > If it is taking the big sleep, then we always wake it up, since > acd4c7d58baf09f. > > I didn't change that part. I only changed what we do if it not hibernating. Right, I hadn't read enough of the context. > It looks like only limited consolidation was happening, the number of kills > invoked was more than half of the number of SetLatch. I think the reason > is what when kill(owner_pid, SIGUSR1) is called, the kernel immediately > suspends the calling process and gives the signalled process a chance to > run in that time slice. The Wal Writer gets woken up, sees that it only > has a partial page to write and decides not to do anything, but resets the > latch. It does this fast enough that the subscription worker hasn't > migrated CPUs yet. I think part of the problem here is that latches signal the owning process even if the owning process isn't actually sleeping - that's obviously quite pointless. In cases where walwriter is busy, that actually causes a lot of superflous interrupted syscalls, because it actually never ends up waiting on the latch. There's also a lot of superflous context switches. I think we can avoid doing so quite easily, as e.g. with the attached patch. Could you check how much that helps your benchmark? > The first change made the WALWriter wake up and do a write and flush > whenever an async commit occurred and there was a completed WAL page to be > written. This way the hint bits could be set while the heap page was still > hot, because they couldn't get set until the WAL covering the hinted-at > transaction commit is flushed. > > The second change said we can set hint bits before the WAL covering the > hinted-at transaction is flushed, as long the page LSN is newer than that > (so the wal covering the hinted-at transaction commit must be flushed > before the page containing the hint bit can be written). > > Then the third change makes the wal writer only write the full pages most > of the times it is woken up, not flush them. It only flushes them once > every wal_writer_delay or wal_writer_flush_after, regardless of how often > it is woken up. > > It seems like the third change rendered the first one useless. I don't think so. Isn't the walwriter writing out the contents of the WAL is quite important because it makes room in wal_buffers for new WAL? I suspect we actually should wake up wal-writer *more* aggressively, to offload wal fsyncs from individual backends, even when they're not committing. Right now we fsync whenever a segment is finished - we really don't want to do that in backends that could do other useful work. I suspect it'd be a good idea to remove that logic from XLogWrite() and replace it with if (ProcGlobal->walwriterLatch) SetLatch(ProcGlobal->walwriterLatch); > Wouldn't it > better, and much simpler, just to have reverted the first change once the > second one was done? I think both can actually happen independently, no? It's pretty common for the page lsn to be *older* than the corresponding commit. In fact you'll always hit that case unless there's also concurrent writes also touching said page. > If that were done, would the third change still be > needed? (It did seem to add some other features as well, so I'm going to > assume we still want those...). It's still useful, yes. It avoids flushing the WAL too often (page-by-page when there's a lot of writes), which can eat up a lot of IOPS on fast storage. > But now the first change is even worse than useless, it is positively > harmful. The only thing to stop it from waking the WALWriter for every > async commit is this line: > > /* if we have already flushed that far, we're done */ > if (WriteRqstPtr <= LogwrtResult.Flush) > return; > > Since LogwrtResult.Flush doesn't advance anymore, this condition doesn't > becomes false due to us waking walwriter, it only becomes false once the > timeout expires (which it would have done anyway with no help from us), or > once wal_writer_flush_after is exceeded. So now every async commit is > waking the walwriter. Most of those wake up do nothing (there is not a > completely new patch to write), some write one completed page but don't > flush anything, and very few do a flush, and that one would have been done > anyway. s/completely new patch/completely new page/? In my opinion we actually *do* want to write (but not flush!) out such pages, so I'm not sure I agree with that logic. Have to think about this some more... Greetings, Andres Freund >From 50a3e62b16b752837f5a388e3259fee2bca6c2ab Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sat, 24 Jun 2017 16:46:11 -0700 Subject: [PATCH 1
Re: [HACKERS] FIPS mode?
To utilize openssl FIPS, you have to explicitly enable it, per the FIPS user guide: https://www.openssl.org/docs/fips/UserGuide-2.0.pdf So, my target would be redhat/centos where openssl FIPS is certified/available, and then add a configuration parameter to enable it (much like Apache HTTPD's SSLFIPS directive: http://httpd.apache.org/docs/current/mod/mod_ssl.html#sslfips). On Sat, Jun 24, 2017 at 1:51 AM Tom Lane wrote: > Michael Paquier writes: > > On Sat, Jun 24, 2017 at 12:56 PM, Curtis Ruck > > wrote: > >> If I clean this up some, maintain styleguide, what is the likely hood of > >> getting this included in the redhat packages, since redhat ships a > certified > >> FIPS implementation? > > > So they are applying a custom patch to it already? > > Don't believe so. It's been a few years since I was at Red Hat, but > my recollection is that their approach was that it was a system-wide > configuration choice changing libc's behavior, and there were only very > minor fixes required to PG's behavior, all of which got propagated > upstream (see, eg, commit 01824385a). It sounds like Curtis is trying > to enable FIPS mode inside Postgres within a system where it isn't enabled > globally, which according to my recollection has basically nothing to do > with complying with the actual federal security standard. > > regards, tom lane >
Re: [HACKERS] FIPS mode?
On 06/23/2017 10:51 PM, Tom Lane wrote: > Michael Paquier writes: >> On Sat, Jun 24, 2017 at 12:56 PM, Curtis Ruck >> wrote: >>> If I clean this up some, maintain styleguide, what is the likely hood of >>> getting this included in the redhat packages, since redhat ships a certified >>> FIPS implementation? > >> So they are applying a custom patch to it already? > > Don't believe so. It's been a few years since I was at Red Hat, but > my recollection is that their approach was that it was a system-wide > configuration choice changing libc's behavior, and there were only very > minor fixes required to PG's behavior, all of which got propagated > upstream (see, eg, commit 01824385a). It sounds like Curtis is trying > to enable FIPS mode inside Postgres within a system where it isn't enabled > globally, which according to my recollection has basically nothing to do > with complying with the actual federal security standard. Yes, see the PostgreSQL DISA STIG for a discussion with respect to that: https://www.crunchydata.com/postgres-stig/PGSQL-STIG-9.5+.pdf HTH, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Code quality issues in ICU patch
Peter Eisentraut writes: > On 6/23/17 12:31, Tom Lane wrote: >> icu_to_uchar() and icu_from_uchar(), and perhaps other places, are >> touchingly naive about integer overflow hazards in buffer size >> calculations. > Here is a patch that should address this. Ah, I was about to suggest the same thing, but I was coming at it from the standpoint of not requiring buffers several times larger than necessary, which could in itself cause avoidable palloc failures. I was going to suggest a small variant actually: run the conversion function an extra time only if the string is long enough to make the space consumption interesting, say if (nbytes < 1024) { /* if it's short, feel free to waste a bit of space */ len_uchar = 2 * nbytes + 1; /* max length per docs */ } else { /* calculate exact space needed */ status = U_ZERO_ERROR; len_uchar = ucnv_toUChars(icu_converter, NULL, 0, buff, nbytes, &status); if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR) ... } *buff_uchar = palloc(len_uchar * sizeof(**buff_uchar)); In this way the extra cycles would seldom be paid in practice. > (I don't think the overruns were exploitable. You'd just get a buffer > overflow error from the ucnv_* function.) Hm, good point. But we might still hit avoidable failures with strings that are a significant fraction of 1GB. 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] Causal reads take II
On 3 January 2017 at 01:43, Thomas Munro wrote: > Here is a new version of my "causal reads" patch (see the earlier > thread from the 9.6 development cycle[1]), which provides a way to > avoid stale reads when load balancing with streaming replication. I'm very happy that you are addressing this topic. I noticed you didn't put in links my earlier doubts about this specific scheme, though I can see doubts from myself and Heikki at least in the URLs. I maintain those doubts as to whether this is the right way forwards. This patch presumes we will load balance writes to a master and reads to a pool of standbys. How will we achieve that? 1. We decorate the application with additional info to indicate routing/write concerns. 2. We get middleware to do routing for us, e.g. pgpool style read/write routing The explicit premise of the patch is that neither of the above options are practical, so I'm unclear how this makes sense. Is there some use case that you have in mind that has not been fully described? If so, lets get it on the table. What I think we need is a joined up plan for load balancing, so that we can understand how it will work. i.e. explain the whole use case and how the solution works. I'm especially uncomfortable with any approaches that treat all sessions as one pool. For me, a server should support multiple pools. Causality seems to be a property of a particular set of pools. e.g. PoolS1 supports causal reads against writes to PoolM1 but not PoolM2, yet PoolS2 does not provide causal reads against PoolM1 orPoolM2. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Code quality issues in ICU patch
On 6/23/17 12:31, Tom Lane wrote: > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > touchingly naive about integer overflow hazards in buffer size > calculations. I call particular attention to this bit in > icu_from_uchar(): > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, > ucnv_getMaxCharSize(icu_converter)); > > The ICU man pages say that that macro is defined as > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) > (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > which means that getting this to overflow (resulting in > probably-exploitable memory overruns) would be about as hard as taking > candy from a baby. Here is a patch that should address this. (I don't think the overruns were exploitable. You'd just get a buffer overflow error from the ucnv_* function.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From c909db0741cb7cdf0b6249e063c7ad78cbf93819 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sat, 24 Jun 2017 09:39:24 -0400 Subject: [PATCH] Refine memory allocation in ICU conversions The simple calculations done to estimate the size of the output buffers for ucnv_fromUChars() and ucnv_toUChars() could overflow int32_t for large strings. To avoid that, go the long way and run the function first without an output buffer to get the correct output buffer size requirement. --- src/backend/utils/adt/pg_locale.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 0f5ec954c3..5478e39ea7 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1506,14 +1506,22 @@ icu_to_uchar(UChar **buff_uchar, const char *buff, size_t nbytes) init_icu_converter(); - len_uchar = 2 * nbytes + 1; /* max length per docs */ - *buff_uchar = palloc(len_uchar * sizeof(**buff_uchar)); status = U_ZERO_ERROR; - len_uchar = ucnv_toUChars(icu_converter, *buff_uchar, len_uchar, + len_uchar = ucnv_toUChars(icu_converter, NULL, 0, + buff, nbytes, &status); + if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR) + ereport(ERROR, + (errmsg("ucnv_toUChars failed: %s", u_errorName(status; + + *buff_uchar = palloc((len_uchar + 1) * sizeof(**buff_uchar)); + + status = U_ZERO_ERROR; + len_uchar = ucnv_toUChars(icu_converter, *buff_uchar, len_uchar + 1, buff, nbytes, &status); if (U_FAILURE(status)) ereport(ERROR, (errmsg("ucnv_toUChars failed: %s", u_errorName(status; + return len_uchar; } @@ -1536,14 +1544,22 @@ icu_from_uchar(char **result, const UChar *buff_uchar, int32_t len_uchar) init_icu_converter(); - len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); + status = U_ZERO_ERROR; + len_result = ucnv_fromUChars(icu_converter, NULL, 0, +buff_uchar, len_uchar, &status); + if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR) + ereport(ERROR, + (errmsg("ucnv_fromUChars failed: %s", u_errorName(status; + *result = palloc(len_result + 1); + status = U_ZERO_ERROR; len_result = ucnv_fromUChars(icu_converter, *result, len_result + 1, buff_uchar, len_uchar, &status); if (U_FAILURE(status)) ereport(ERROR, (errmsg("ucnv_fromUChars failed: %s", u_errorName(status; + return len_result; } #endif /* USE_ICU */ -- 2.13.1 -- Sent 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 a typo in snapmgr.c
On 23 June 2017 at 19:25, Andres Freund wrote: > On 2017-06-23 19:21:57 +0100, Simon Riggs wrote: >> On 23 June 2017 at 08:23, Simon Riggs wrote: >> > On 23 June 2017 at 08:21, Andres Freund wrote: >> >> On 2017-06-07 10:17:31 -0700, Andres Freund wrote: >> >>> On 2017-05-08 09:12:13 -0400, Tom Lane wrote: >> >>> > Simon Riggs writes: >> >>> > > So rearranged code a little to keep it lean. >> >>> > >> >>> > Didn't you break it with that? As it now stands, the memcpy will >> >>> > copy the nonzero value. >> >>> >> >>> I've not seen a fix and/or alleviating comment about this so far. Did I >> >>> miss something? >> >> >> >> Simon, FWIW, I plan to either revert or fix this up soon-ish. Unless >> >> you're going to actually respond on this thread? >> > >> > Sorry, I've only just seen Tom's reply. Will fix. >> >> I don't see a bug. Perhaps I'm tired and can't see it yet. >> >> Will fix if you thwack me with the explanation. > > Wasn't my complaint, but here we go: > > Previous code: > > /* > * Ignore the SubXID array if it has overflowed, unless the snapshot > was > * taken during recovey - in that case, top-level XIDs are in subxip > as > * well, and we mustn't lose them. > */ > if (serialized_snapshot.suboverflowed && > !snapshot->takenDuringRecovery) > serialized_snapshot.subxcnt = 0; > > /* Copy struct to possibly-unaligned buffer */ > memcpy(start_address, >&serialized_snapshot, sizeof(SerializedSnapshotData)); > > i.e. if suboverflowed, start_address would contain subxcnt = 0. > > New code: > > > /* Copy struct to possibly-unaligned buffer */ > memcpy(start_address, >&serialized_snapshot, sizeof(SerializedSnapshotData)); > > /* Copy XID array */ > if (snapshot->xcnt > 0) > memcpy((TransactionId *) (start_address + > > sizeof(SerializedSnapshotData)), >snapshot->xip, snapshot->xcnt * > sizeof(TransactionId)); > > /* > * Copy SubXID array. Don't bother to copy it if it had overflowed, > * though, because it's not used anywhere in that case. Except if > it's a > * snapshot taken during recovery; all the top-level XIDs are in > subxip as > * well in that case, so we mustn't lose them. > */ > if (serialized_snapshot.suboverflowed && > !snapshot->takenDuringRecovery) > serialized_snapshot.subxcnt = 0; > > Here the copy is done before subxcnt = 0. OK, me looking at the wrong memcpy, my bad. Thanks for the thwack. Fixed. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Modifing returning value of PQgetvalue.
Hello, PQgetvalue returns a value of type char* (without const). But the documentation says: "The pointer returned by PQgetvalue points to storage that is part of the PGresult structure. *One should not modify the data it points to*" (my italics). Could someone tell me please, what wrong with modifing arbitrary character of the data pointed by PQgetvalue's returning value? Or why this restriction is documented? Thanks.