Re: [HACKERS] transition table behavior with inheritance appears broken

[Noah Misch]

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

[Alvaro Herrera]

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

[Tatsuo Ishii]

> 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

[Tatsuo Ishii]

> 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

[Andres Freund]

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 

Re: [HACKERS] FIPS mode?

[Curtis Ruck]

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?

[Joe Conway]

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

[Tom Lane]

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, );
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

[Simon Riggs]

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

[Peter Eisentraut]

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, 
);
+   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, 
);
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, );
+   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, );
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

[Simon Riggs]

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,
>_snapshot, sizeof(SerializedSnapshotData));
>
> i.e. if suboverflowed, start_address would contain subxcnt = 0.
>
> New code:
>
>
> /* Copy struct to possibly-unaligned buffer */
> memcpy(start_address,
>_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.

[Dmitry Igrishin]

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.