Re: [HACKERS] New partitioning - some feedback

[Amit Langote]

On 2017/07/08 14:12, Mark Kirkwood wrote:
> On 07/07/17 19:54, Michael Banck wrote:
>> On Fri, Jul 07, 2017 at 07:40:55PM +1200, Mark Kirkwood wrote:
>>> On 07/07/17 13:29, Amit Langote wrote:
 Someone complained about this awhile back [1].  And then it came up again
 [2], where Noah appeared to take a stance that partitions should be
 visible in views / output of commands that list "tables".

 Although I too tend to prefer not filling up the \d output space by
 listing partitions (pg_class.relispartition = true relations), there
 wasn't perhaps enough push for creating a patch for that.  If some
 committer is willing to consider such a patch, I can make one.
>>>
>>> Yeah, me too (clearly). However if the consensus is that all these
>>> partition
>>> tables *must* be shown in \d output, then I'd be happy if they were
>>> identified as such rather than just 'table' (e.g 'partition table').
>> +1.
>>
>> Or maybe just 'partition' is enough if 'partition table' would widen the
>> column output unnecessarily.
> 
> Yeah, that is probably better (and 'partition table' is potentially
> confusing as Robert pointed out).

So, it seems at least that showing "partition" as the Type of tables that
are actually partitions is preferable.  I created a patch (attached 0001)
that implements that.  It makes \d show any relations that have
relispartition = true as of type "partition" or "foreign partition".  With
the patch:

create table p (a int) partition by list (a);

-- regular table as partition
create table p1 partition of p for values in (1)

-- foreign table as partition
create foreign data wrapper dummy;
create server dummy foreign data wrapper dummy;
create foreign table p2 partition of p for values in (2) server dummy;

-- partitioned table as partition
create table p3 partition of p for values in (3) partition by list (a);

\d
 List of relations
 Schema | Name |   Type| Owner
+--+---+---
 public | p| table | amit
 public | p1   | partition | amit
 public | p2   | foreign partition | amit
 public | p3   | partition | amit
(4 rows)

Also, there seems to be at least some preference for excluding partitions
by default from the \d listing.  Attached 0002 implements that.  To enable
showing partitions, the patch adds a new modifier '!' to \d (picked '!'
from Robert's email on this thread [1]).  With the patch:

\d
   List of relations
 Schema | Name | Type  | Owner
+--+---+---
 public | p| table | amit
(1 row)

\d!
 List of relations
 Schema | Name |   Type| Owner
+--+---+---
 public | p| table | amit
 public | p1   | partition | amit
 public | p2   | foreign partition | amit
 public | p3   | partition | amit
(4 rows)

\d!+
 List of relations
 Schema | Name |   Type| Owner |  Size   | Description
+--+---+---+-+-
 public | p| table | amit  | 0 bytes |
 public | p1   | partition | amit  | 0 bytes |
 public | p2   | foreign partition | amit  | 0 bytes |
 public | p3   | partition | amit  | 0 bytes |
(4 rows)

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoYNPHFjY%2BObFF9%3DTbX%2BT6ez1FAU%2BsmGuXeoiOMasDc-0g%40mail.gmail.com
From c73da2fcfc81ffa351f96be000ae5d262d828ae1 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 10 Jul 2017 13:25:20 +0900
Subject: [PATCH 1/2] Show "(foreign) partition" as Type in \d output

---
 src/bin/psql/describe.c | 48 +++-
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e6833eced5..bbdac8d50d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3321,27 +3321,57 @@ listTables(const char *tabtypes, const char *pattern, 
bool verbose, bool showSys
printfPQExpBuffer(,
  "SELECT n.nspname as \"%s\",\n"
  "  c.relname as \"%s\",\n"
- "  CASE c.relkind"
- " WHEN " 
CppAsString2(RELKIND_RELATION) " THEN '%s'"
+ "  CASE c.relkind",
+ gettext_noop("Schema"),
+ gettext_noop("Name"));
+
+   /*
+* Starting in PG 10, certain kinds of relations could be partitions, 
which
+* if so, we show Type accordingly.
+*/
+   if (pset.sversion >= 10)
+   appendPQExpBuffer(,
+ " WHEN " 
CppAsString2(RELKIND_RELATION) " THEN"
+ "   CASE c.relispartition"
+  

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Etsuro Fujita]

On 2017/07/07 18:47, Amit Langote wrote:

On 2017/07/06 16:06, Etsuro Fujita wrote:

I think this should be fixed.  Attached is a patch for that.


Looking up the ResultRelInfo using the proposed method in
ExecFindResultRelInfo() can be unreliable sometimes, that is, the method
of using the root table OID to pin down the RangeTableEntry to get the the
modified columns set.


You are right.  Thank you for pointing that out.


How about setting ri_RangeTableIndex of the partition ResultRelInfo
correctly in the first place?  If you look at the way InitResultRelInfo()
is called in ExecSetupPartitionTupleRouting(), a dummy RT index of 1 is
passed.  We could instead pass the correct one.  I think
ModifyTable.nominalRelation is that (at least in the INSERT case.

The attached patch implements that.  It modifies
ExecSetupPartitionTupleRouting to accept one more argument resultRTindex
and passes along the same to InitResultRelInfo().  Later when
ExecConstraints() wants to build the modifiedCols set, it looks up the
correct RTE using the partition ResultRelInfo, which has the appropriate
ri_RangeTableIndex set and uses the same.


Looks good to me.


The patch keeps tests that you added in your patch.  Added this to the
open items list.


Another thing I noticed is the error handling in ExecWithCheckOptions; 
it doesn't take any care for partition tables, so the column description 
in the error message for WCO_VIEW_CHECK is built using the partition 
table's rowtype, not the root table's.  But I think it'd be better to 
build that using the root table's rowtype, like ExecConstraints, because 
that would make the column description easier to understand since the 
parent view(s) (from which WithCheckOptions evaluated there are created) 
would have been defined on the root table.  This seems independent from 
the above issue, so I created a separate patch, which I'm attaching. 
What do you think about that?


Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 2097,2102  ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
--- 2097,2121 
 * USING policy.
 */
case WCO_VIEW_CHECK:
+   /* See the comment in 
ExecConstraints(). */
+   if (resultRelInfo->ri_PartitionRoot)
+   {
+   HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
+   TupleDesc   old_tupdesc = 
RelationGetDescr(rel);
+   TupleConversionMap *map;
+ 
+   rel = 
resultRelInfo->ri_PartitionRoot;
+   tupdesc = RelationGetDescr(rel);
+   /* a reverse map */
+   map = 
convert_tuples_by_name(old_tupdesc, tupdesc,
+   
 gettext_noop("could not convert row type"));
+   if (map != NULL)
+   {
+   tuple = 
do_convert_tuple(tuple, map);
+   ExecStoreTuple(tuple, 
slot, InvalidBuffer, false);
+   }
+   }
+ 
insertedCols = 
GetInsertedColumns(resultRelInfo, estate);
updatedCols = 
GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, 
updatedCols);
*** a/src/test/regress/expected/updatable_views.out
--- b/src/test/regress/expected/updatable_views.out
***
*** 2424,2429  select tableoid::regclass, * from pt;
  create view ptv_wco as select * from pt where a = 0 with check option;
  insert into ptv_wco values (1, 2);
  ERROR:  new row violates check option for view "ptv_wco"
! DETAIL:  Failing row contains (2, 1).
  drop view ptv, ptv_wco;
  drop table pt, pt1, pt11;
--- 2424,2429 
  create view ptv_wco as select * from pt where a = 0 with check option;
  insert into ptv_wco values (1, 2);
  ERROR:  new row violates check option for view "ptv_wco"
! DETAIL:  Failing row contains (1, 2).
  drop view ptv, ptv_wco;
  drop table pt, pt1, pt11;

-- 
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] hash index on unlogged tables doesn't behave as expected

[Kyotaro HORIGUCHI]

At Sat, 8 Jul 2017 16:41:27 +0530, Amit Kapila  wrote 
in 
> On Sat, Jul 8, 2017 at 9:06 AM, Amit Kapila  wrote:
> > On Sat, Jul 8, 2017 at 8:52 AM, Robert Haas  wrote:
> >> On Thu, Jul 6, 2017 at 5:54 AM, Kyotaro HORIGUCHI
> >>  wrote:
> >>> Check for INIT_FORKNUM appears both accompanied and not
> >>> accompanied by check for RELPER.._UNLOGGED, so I'm not sure which
> >>> is the convention here.
> >>
> >> Checking only for INIT_FORKNUM seems logical to me.  Also checking for
> >> RELPERSISTENCE_UNLOGGED just makes the code longer to no benefit.  I
> >> guess Amit copied the test from ATExecSetTablespace, which does it as
> >> he did, but it seems unnecessarily long-winded.
> >>
> >
> > Okay.  If you and Michael feel the check that way is better, I will
> > change and submit the patch.
> >
> 
> Changed as per suggestion.


> On Sat, Jul 8, 2017 at 9:08 AM, Robert Haas  wrote:
> > On Fri, Jul 7, 2017 at 11:36 PM, Amit Kapila  
> > wrote:
> >> I think we should do that as a separate patch (I can write the same as
> >> well) because that is not new behavior introduced by this patch, ...
> >
> > True, although maybe hash indexes are the only way that happens today?
> >
> 
> No, I think it will happen for all other indexes as well.  Basically,
> we log new page for init forks of other indexes and then while
> restoring that full page image, it will fall through the same path.

(Sorry, I didn't noticed that hash metapage is not using log_newpgae)

For example, (bt|gin|gist|spgist|brin)buildempty are using
log_newpage to log INIT_FORK metapages. I believe that the xlog
flow from log_newpage to XLogReadBufferForRedoExtended is
developed so that pages in init forks are surely flushed during
recovery, so we should fix it instead adding other flushes if the
path doesn't work. Am I looking wrong place? (I think it is
working.)

If I'm understanding correctly here, hashbild and hashbuildempty
should be refactored using the same structure with other *build
and *buildempty functions. Specifically metapages initialization
subroutines (_hash_init or _bt_initmetapage or SpGistInitMetapage
or...)  does only on-memory initialization and does not emit WAL,
then *build and *buildempty emits WAL in their required way.


> >>> By the way the comment of the function ReadBufferWithoutRelcache
> >>> has the following sentense.
> >>>
> >>> | * NB: At present, this function may only be used on permanent 
> >>> relations, which
> >>> | * is OK, because we only use it during XLOG replay.  If in the future we
> >>> | * want to use it on temporary or unlogged relations, we could pass 
> >>> additional
> >>> | * parameters.
> >>>
> >>> and does
> >>>
> >>> | return ReadBuffer_common(smgr, RELPERSISTENCE_PERMANENT, forkNum, 
> >>> blockNum,
> >>>  mode, strategy, 
> >>> );
> >>>
> >>> This surely works since BufferAlloc recognizes INIT_FORK. But
> >>> some adjustment may be needed around here.
> >>
> >> Yeah, it should probably mention that the init fork of an unlogged
> >> relation is also OK.
> >>
> >
> > I think we should do that as a separate patch (I can write the same as
> > well) because that is not new behavior introduced by this patch, but
> > let me know if you think that we should add such a comment in this
> > patch itself.
> >
> 
> Attached a separate patch to adjust the comment atop 
> ReadBufferWithoutRelcache.

+ * NB: At present, this function may only be used on permanent relations and
+ * init fork of an unlogged relation, which is OK, because we only use it
+ * during XLOG replay.  If in the future we want to use it on temporary or
+ * unlogged relations, we could pass additional parameters.

*I* think the target of the funcion is permanent relations and
init forks, not unlogged relations. And I'd like to have an
additional comment about RELPERSISTENCE_PERMANENT. Something like
the attached.

reagards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 15795b0..cc3980c 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -673,10 +673,10 @@ ReadBufferExtended(Relation reln, ForkNumber forkNum, BlockNumber blockNum,
  * ReadBufferWithoutRelcache -- like ReadBufferExtended, but doesn't require
  *		a relcache entry for the relation.
  *
- * NB: At present, this function may only be used on permanent relations, which
- * is OK, because we only use it during XLOG replay.  If in the future we
- * want to use it on temporary or unlogged relations, we could pass additional
- * parameters.
+ * NB: At present, this function may only be used on main fork pages of
+ * permanent relations and init fork pages, which is OK, 

Re: [HACKERS] POC: Cache data in GetSnapshotData()

[Mithun Cy]

On Fri, Apr 8, 2016 at 12:13 PM, Robert Haas  wrote:
> I think that we really shouldn't do anything about this patch until
> after the CLOG stuff is settled, which it isn't yet.  So I'm going to
> mark this Returned with Feedback; let's reconsider it for 9.7.

I am updating a rebased patch have tried to benchmark again could see
good improvement in the pgbench read-only case at very high clients on
our cthulhu (8 nodes, 128 hyper thread machines) and power2 (4 nodes,
192 hyper threads) machine. There is some issue with base code
benchmarking which is somehow not consistent so once I could figure
out what is the issue with that I will update

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


Cache_data_in_GetSnapshotData_POC_02.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] [PATCH] Minor typo in the source repository documentation

[Tom Lane]

Martin Mai  writes:
> Tom Lane  wrote:
>> I was considering changing "the files they are used to build"
>> to "the files that these tools are used to build".  I think the
>> main problem is just misunderstanding which things "they" means,
>> and that should fix it.

> +1
> Just after writing my last message it clicked and I understood that the
> current wording is correct. Your suggestion makes it clearer, though.

Done that way.  Thanks for the suggestion!

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] SCRAM auth and Pgpool-II

[Michael Paquier]

On Sat, Jul 8, 2017 at 1:54 PM, Álvaro Hernández Tortosa  
wrote:
> There's definitely an important concern here that should be addressed:
> how poolers/proxies/middleware/etc can deal with SCRAM, specifically in the
> context of channel binding.
>
> If there is to be a single connection from client to PostgreSQL server,
> intercepted by pgpool to perform the magic foo, then channel binding is,
> indeed, designed to defeat this. If, however, pgpool or the middleware
> manages two separate connections (client<->pool and pool<->PG) then there is
> some light here.

Thanks. You are putting is more simple words the concepts I am coming
at. One issue is how to send the password to pgpool, which needs to
have it in cleartext for the SASL exchange with each Postgres backend.

> One SCRAM feature not implemented as of today is the authzid
> (authorization identity: see https://tools.ietf.org/html/rfc5802#page-10,
> SCRAM attribute "a" and https://tools.ietf.org/html/rfc5801). Authzid is
> basically "I want to authenticate as user X and once authenticated, consider
> I'm user Y". With authzid, a pool/proxy may have a common user name with its
> own SCRAM credentials to authenticate with the backend PostgreSQL, and pass
> the authzid with the real username (the one provided on the client<->pool
> connection).

This RFC paragraph is relevant as well:
https://tools.ietf.org/html/rfc4422#section-2. Nothing will happen in
PG10 regarding that part.

> This would require:
>
> a) That authzid is implemented in PostgreSQL.
> b) A mechanism in PG to name which user(s) Y are allowed to be authenticated
> by user X. This is similar, but not identical, to the current SET ROLE.

A more granular SET SESSION AUTHORIZATION then?
-- 
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] SCRAM auth and Pgpool-II

[Michael Paquier]

On Sat, Jul 8, 2017 at 9:30 AM, Stephen Frost  wrote:
> Michael,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> On Sat, Jul 8, 2017 at 1:24 AM, Robert Haas  wrote:
>> > IIUC, things will get even worse once channel binding is committed,
>> > presumably for PostgreSQL 11.  The point of channel binding is to
>> > guarantee that you are conducting the authentication exchange with the
>> > target server, not some intermediate proxy that might be conducting a
>> > hostile MITM attack.  pgpool may not be a hostile attack, but it is
>> > acting as a MITM, and channel binding is going to figure that out and
>> > fail the authentication.  So unless I'm misunderstanding, the solution
>> > you are proposing figures to have a very limited shelf life.
>>
>> We may be misunderstanding each other then. The solution I am
>> proposing, or at least the solution that I imagine should be done but
>> my pgpool-fu is limited, is to keep around connection context and SSL
>> context to handle properly connections messages, and switch between
>> them. When using channel binding, the client needs the server
>> certificate for end-point and the TLS finish message which are both
>> stored in the SSL context after the handshake. So you need to cache
>> that for each server.
>
> I'm not sure what you mean here- the whole point of channel binding is
> to prevent exactly the kind of MITM that pgPool would essentially be in
> this case.  If you're suggesting that things be changed such that a
> server provides information necessary to pgPool to pass along to the
> client to make the client believe that it's talking to the server and
> that there's no man-in-the-middle then the whole point of channel
> binding goes out the window.  If what you're suggesting doesn't require
> any help from either the client or the server to work then I fear we've
> done something wrong in the implementation of channel binding (I've not
> looked at it very closely).

Sorry if my last reply sounded confusing. I have re-read the thread,
my replies and what pgpool does and I can see now what can be
confusing in my words.

What I am suggesting here is that in order to handle properly SCRAM
with channel binding, pgpool has to provide a different handling for
client <-> pgpool and pgpool <-> Postgres. In short, I don't have a
better answer than having pgpool impersonate the server and request
for a password in cleartext through an encrypted connection between
pgpool and the client if pgpool does not know about it, and then let
pgpool do by itself the SCRAM authentication on a per-connection basis
with each Postgres instances. When using channel binding, what would
matter is the TLS finish (for tls-unique) or the hash server
certificate between Postgres and pgpool, not between the client and
pgpool. But that's actually the point you are raising here:

> I don't have any perfect answers for pgPool here, unfortunately.  One
> approach would be to give it a list of usernames/passwords to use,
> though that's hardly ideal.  Perhaps it would be possible for pgPool to
> impersonate the server to the client and the client to the server if it
> was able to query the database as a superuser on some other connection,
> but I don't *think* so because of the way SCRAM works and because pgPool
> wouldn't have access to the client's cleartext password.
>
> Of course, if pgPool could convince the client to use 'password' auth
> and get the cleartext password from the client then that would work to
> authenticate against the server and there wouldn't be any channel
> binding involved since the client is talking the basic cleartext
> password protocol and not SCRAM.

Yes, that's what I am coming at. The only way to defeat a password
sent to pgpool in cleartext would be to force the client to do things
through an encrypted connection.

>> One problem is that pgpool does not use directly libpq but tries to
>> handle things by itself at protocol level, so it needs to replicate a
>> lot of existing APIs. Postgres-XC/XL use a connection pooler,
>> presumably XL uses even a background worker for this stuff since it
>> has been integrated with Postgres 9.3. And this stuff use libpq. This
>> makes life way easier to handle context data on a connection basis.
>> Those don't need to handle protocol-level messages either, which is
>> perhaps different from what pgpool needs.
>
> I don't really think that pgpool using or not using libpq makes any
> real difference here.  What'll be an issue for pgpool is when clients
> get updated libpq libraries that don't support password auth...

There is no need to duplicate the protocol level handling, so life is
simplified for pgpool. But that cannot happen as Ishii-san said
upthread as pgpool needs to control things at a higher level.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server

[Michael Paquier]

On Sat, Jul 8, 2017 at 12:50 AM, Masahiko Sawada  wrote:
> On Fri, Jul 7, 2017 at 3:48 PM, Michael Paquier
>  wrote:
>> So I would suggest the following things to address this issue:
>> 1) Generate a backup history file for backups taken at recovery as well.
>> 2) Archive it if archive_mode = always.
>> 3) Wait for both the segment of the stop point and the backup history
>> files to be archived before returning back to the caller of
>> pg_stop_backup.
>>
>> It would be nice to get all that addressed in 10. Thoughts?
>
> Yeah, I agree. Attached patch makes it works and deals with the
> history file issue.

I had a look at the proposed patch. Here are some comments.

@@ -11002,10 +11000,10 @@ do_pg_stop_backup(char *labelfile, bool
waitforarchive, TimeLineID *stoptli_p)
if (waitforarchive && XLogArchivingActive())
{
XLByteToPrevSeg(stoppoint, _logSegNo);
-   XLogFileName(lastxlogfilename, ThisTimeLineID, _logSegNo);
+   XLogFileName(lastxlogfilename, stoptli, _logSegNo);

On a standby the wait phase should not happen if archive_mode = on,
but only if archive_mode = always. So I would suggest to change this
upper condition a bit, and shuffle a bit the code to make the wait
phase happen last:
1) stoptli_p first.
2) Check for XLogArchivingActive or XLogArchivingAlways, then with the
NOTICE message.
3) Do the actual wait.
This way the code doing the wait does not need to be in its long
lengthy if() branch. I think that we should replace the pg_usleep()
call with a latch to make this more responsive. That should be a
future patch.

In backup history files generated in standbys, the STOP TIME is not
set and this results in garbage in the file.

+If second parameter is true and on standby, pg_stop_backup
+waits for WAL to be archived without forcibly switching WAL on standby.
+So enforcing manually a WAL switch on primary needs to happen.
Here is a reformulation:
If the second parameter wait_for_archive is true and the backup is
taken on a standby, pg_stop_backup waits for WAL to be archived when
archive_mode = always. Enforcing manually a WAL segment switch to
happen with for example pg_switch_wal() may be necessary if the
primary has low activity to allow the backup to complete. Using
statement_timeout to limit the amount of time to wait or switching
wait_for_archive to false will control the wait time, though all the
WAL segments necessary to recover into a consistent state from the
backup taken may not be archived at the time pg_stop_backup returns
its status to the caller.

The errhint() for the wait phase should be reworked for a standby. I
would suggest for errmsg() the same thing, aka:
pg_stop_backup still waiting for all required WAL segments to be
archived (%d seconds elapsed)
But the following for a backup started in recovery. That's long but
things need to be really clear to the user:
Backup has been taken from a standby, check if the WAL segments needed
for this backup have been completed, in which case a forced segment
switch may can be needed on the primary. If a segment switch has
already happened check that your archive_command is executing
properly. pg_stop_backup can be canceled safely, but the database
backup will not be usable without all the WAL segments.
-- 
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_receivewal and messages printed in non-verbose mode

[Michael Paquier]

On Sun, Jul 9, 2017 at 9:30 PM, Michael Paquier
 wrote:
> On Sun, Jul 9, 2017 at 5:58 AM, Magnus Hagander  wrote:
>> I wonder if we should actually just remove the second message? AFAICT no
>> other tools log that information. Is there any particular reason why we want
>> that logging in pg_receivewal when we don't have it in other tools?
>
> I maintain a fork of pg_receivewal lately that works as a service, and
> I have found myself a fan of this log bit when debugging funky issues.
> That's a personal opinion, no objections to remove it either.

The patch I sent upthread was actually doing that, which is obviously incorrect:
-if (time_to_abort)
+if (verbose && time_to_abort)
 {
 fprintf(stderr, _("%s: received interrupt signal, exiting\n"),
 progname);

While the version on my laptop does that:
if (time_to_abort)
{
-   fprintf(stderr, _("%s: received interrupt signal, exiting\n"),
-   progname);
+   if (verbose)
+   fprintf(stderr, _("%s: received interrupt
signal, exiting\n"),
+   progname);
return true;
}
return false;
Not sure how that feel into the cracks.

I slept on it, and let's do things the same way as the other tools do
without logs in this case. So this gives the patch attached.
-- 
Michael


receivewal-verbose-fix-v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] replication_slot_catalog_xmin not explicitly initialized when creating procArray

[Masahiko Sawada]

On Sat, Jul 8, 2017 at 2:19 AM, Wong, Yi Wen  wrote:
> Hi,
>
>
> replication_slot_catalog_xmin is not explictly initialized to
> InvalidTransactionId.
>
>
> Normally, there isn't an issue with this because a freshly mmap'd memory is
> zeroed, and the value of InvalidTransactionId is 0.
>
> If the memory was not 0 for whatever reason, VACUUM would not behave as
> expected.
>
>
> See attached patch.
>

Thank you for the patch. This change makes sense to me.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] Authentification method on client side checking

[Michael Paquier]

On Mon, Jul 10, 2017 at 9:29 AM, Álvaro Hernández Tortosa
 wrote:
> Precisely yesterday I initiated a similar thread:
> https://www.postgresql.org/message-id/d4098ef4-2910-c8bf-f1e3-f178ba77c381%408kdata.com
>
> I think that a) the mere auth mechanism is not enough (channel binding
> or not, ssl or not, change a lot the effective security obtained) and b)
> maybe a categorization is a better way of specifying a connection security
> requirements.
>
> What's your opinion on this? Any answer should also be coordinated among
> the drivers.

Before rushing into implementing something that we may not want, let's
discuss the matter on the thread spawned by Álvaro and find an
agreement and a direction of implementation. I was planning to answer
your message with my own thoughts on the matter. Having more control
in libpq is definitely something that we should have.
-- 
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] Fix header comment of streamutil.c

[Masahiko Sawada]

On Fri, Jul 7, 2017 at 9:09 PM, Magnus Hagander  wrote:
> On Fri, Jul 7, 2017 at 8:31 AM, Masahiko Sawada 
> wrote:
>>
>> Hi,
>>
>> While reading source code, I found that the header comment of
>> streamutil.c is not correct. I guess pg_receivelog is a mistake of
>> pg_receivexlog and it's an oversight when changing xlog to wal.
>>
>> Attached patch fixes this.
>
>
> Yeah, both a typo and a missing user :)
>
> Applied, thanks.
>

Thank you!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] Minor typo in the source repository documentation

[Martin Mai]

+1

Just after writing my last message it clicked and I understood that the
current wording is correct. Your suggestion makes it clearer, though.

Tom Lane  wrote:

> I was considering changing "the files they are used to build"
> to "the files that these tools are used to build".  I think the
> main problem is just misunderstanding which things "they" means,
> and that should fix it.
>

Re: [HACKERS] [PATCH] Minor typo in the source repository documentation

[Martin Mai]

Oh, now I think I got it...

The tools are not required to build from distribution tarball, because
the files that are normally build by the tools, are already included
in the tarball.

Realizing that, I agree with you that the current wording is correct
and my second suggestion would be as wrong as my first.

My bad.

Cheers
Martin Mai

Re: [HACKERS] Authentification method on client side checking

[Álvaro Hernández Tortosa]

On 09/07/17 18:47, Victor Drobny wrote:

Hello,

Despite the addition of SCRAM authentification to PostgreSQL 10, MITM 
attack can be performed by saying that the server supports, for 
example, only md5 authentication. The possible solution for it is 
checking authentification method on a client side and reject 
connections that could be unsafe.


Postgresql server can require unencrypted password passing, md5, 
scram, gss or sspi authentification.


Hi Victor.

Precisely yesterday I initiated a similar thread: 
https://www.postgresql.org/message-id/d4098ef4-2910-c8bf-f1e3-f178ba77c381%408kdata.com


I think that a) the mere auth mechanism is not enough (channel 
binding or not, ssl or not, change a lot the effective security 
obtained) and b) maybe a categorization is a better way of specifying a 
connection security requirements.


What's your opinion on this? Any answer should also be coordinated 
among the drivers.



Álvaro


--

Álvaro Hernández Tortosa


---
<8K>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] COPY vs. transition tables

[Thomas Munro]

On Sun, Jul 9, 2017 at 11:46 AM, Thomas Munro
 wrote:
> On Sat, Jul 8, 2017 at 8:42 PM, David Fetter  wrote:
>> Using the script attached, I'm getting this very odd result set below.
>>
>> Audit records from COPY to the "foo bar" table aren't getting
>> recorded, but audit records from COPY to the baz table are.
>
> Thanks for the bug report.  I think it's the presence of the index on
> "foo bar", not the space in its name (but thanks for that curve
> ball!), that causes these tuples not to be captured.
> CopyFromInsertBatch takes a different path depending on whether there
> are any indexes, and mistakenly passes NULL for transition_capture.
> The attached seems to fix it, but I'll look more closely and send a
> version with a regression test on Monday.

Here it is.  Added to open items.

-- 
Thomas Munro
http://www.enterprisedb.com


copy-with-indexes-and-transitions-v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Minor typo in the source repository documentation

[Tom Lane]

Pantelis Theodosiou  writes:
> On Sun, Jul 9, 2017 at 6:27 PM, Martin Mai  wrote:
>> Since "they" means the tools, would changing
>> ... the files they are used to build are included in the tarball.
>> to
>> ... the files they use to build [it] are included in the tarball.
>> convey the intended meaning more clearly?

> Perhaps:
> .. since the necessary files are included in the tarball.

I think that might give the impression that we'd bundled Perl et al
into the tarball :-(

I was considering changing "the files they are used to build"
to "the files that these tools are used to build".  I think the
main problem is just misunderstanding which things "they" means,
and that should fix it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Minor typo in the source repository documentation

[Pantelis Theodosiou]

On Sun, Jul 9, 2017 at 6:27 PM, Martin Mai  wrote:

> Hello Tom,
>
> thanks for the clarification. The sentence felt a little bumpy to me,
> but I am native German speaker, so maybe it is just me then.
>
> Since "they" means the tools, would changing
> ... the files they are used to build are included in the tarball.
> to
> ... the files they use to build [it] are included in the tarball.
> convey the intended meaning more clearly?
>

Perhaps:

.. since the necessary files are included in the tarball.

Re: [HACKERS] [PATCH] Minor typo in the source repository documentation

[Martin Mai]

Hello Tom,

thanks for the clarification. The sentence felt a little bumpy to me,
but I am native German speaker, so maybe it is just me then.

Since "they" means the tools, would changing
... the files they are used to build are included in the tarball.
to
... the files they use to build [it] are included in the tarball.
convey the intended meaning more clearly?

If not, that is fine and I am out of your hair :)
Thanks for your time!

Cheers
Martin Mai

On Sun, Jul 9, 2017 at 6:28 PM, Tom Lane  wrote:

> Martin Mai  writes:
> > I found a typo while reading the source repository documentation:
> > https://www.postgresql.org/docs/devel/static/sourcerepo.html
>
> Hm, I dunno, I like the existing wording better than yours.  I agree
> that it's a bit unclear that the antecedent of "they" is meant to be
> the tools not the files; but this phraseology seems to make the files
> be the active agents, which is just weird.
>
> regards, tom lane
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alexander Korotkov]

On Sun, Jul 9, 2017 at 1:11 PM, Mark Rofail  wrote:

> On Sun, Jul 9, 2017 at 2:38 AM, Alexander Korotkov 
> wrote:
>
>> Could you, please, specify idea of what you're implementing in more
>> detail?
>>
>
> Ultimatley we would like an indexed scan instead of a sequential scan, so
> I thought we needed to index the FK array columns first.
>

Indeed, this is right.
But look how that works for regular FK.  When you declare a FK, you
necessary need unique index on referenced column(s).  However, index on
referencing columns(s) is not required.  Without index on referencing
column(s), row delete in referenced table and update of referenced column
are expensive because requires sequential scan of referencing table.  Users
are encouraged to index referencing column(s) to accelerate queries
produced by RI triggers. [1]
According to this, it's unclear why array FKs should behave differently.
We may document that GIN index is required to accelerate RI queries for
array FKs.  And users are encouraged to manually define them.
It's also possible to define new option when index on referencing column(s)
would be created automatically.  But I think this option should work the
same way for regular FKs and array FKs.

1.
https://www.postgresql.org/docs/current/static/ddl-constraints.html#DDL-CONSTRAINTS-FK

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

[HACKERS] Authentification method on client side checking

[Victor Drobny]

Hello,

Despite the addition of SCRAM authentification to PostgreSQL 10, MITM 
attack can be performed by saying that the server supports, for example, 
only md5 authentication. The possible solution for it is checking 
authentification method on a client side and reject connections that 
could be unsafe.


Postgresql server can require unencrypted password passing, md5, scram, 
gss or sspi authentification.


In the attached patch you can find the solution for it. The new provided 
features are the following:
The parameter with acceptable authentification methods can be passed 
into connection methods of libpq library.
Also, this parameter can be specified to psql as a command line 
argument.
The documentation for command line arguments of psql and arguments of 
libpq methods are also presented.


Thank you for attention!

Best,
--
--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 8068a28..1877b2d 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -75,6 +75,7 @@ struct adhoc_opts
 	bool		no_psqlrc;
 	bool		single_txn;
 	bool		list_dbs;
+	char	   *authtype;
 	SimpleActionList actions;
 };
 
@@ -213,7 +214,7 @@ main(int argc, char *argv[])
 	/* loop until we have a password if requested by backend */
 	do
 	{
-#define PARAMS_ARRAY_SIZE	8
+#define PARAMS_ARRAY_SIZE	9
 		const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
 		const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
 
@@ -232,8 +233,10 @@ main(int argc, char *argv[])
 		values[5] = pset.progname;
 		keywords[6] = "client_encoding";
 		values[6] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto";
-		keywords[7] = NULL;
-		values[7] = NULL;
+		keywords[7] = "acc_auth";
+		values[7] = options.authtype;
+		keywords[8] = NULL;
+		values[8] = NULL;
 
 		new_pass = false;
 		pset.db = PQconnectdbParams(keywords, values, true);
@@ -441,6 +444,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts * options)
 		{"single-line", no_argument, NULL, 'S'},
 		{"tuples-only", no_argument, NULL, 't'},
 		{"table-attr", required_argument, NULL, 'T'},
+		{"authtype", required_argument, NULL, 'u'},
 		{"username", required_argument, NULL, 'U'},
 		{"set", required_argument, NULL, 'v'},
 		{"variable", required_argument, NULL, 'v'},
@@ -458,7 +462,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts * options)
 
 	memset(options, 0, sizeof *options);
 
-	while ((c = getopt_long(argc, argv, "aAbc:d:eEf:F:h:HlL:no:p:P:qR:sStT:U:v:VwWxXz?01",
+	while ((c = getopt_long(argc, argv, "aAbc:d:eEf:F:h:HlL:no:p:P:qR:sStT:u:U:v:VwWxXz?01",
 			long_options, )) != -1)
 	{
 		switch (c)
@@ -566,6 +570,9 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts * options)
 			case 'T':
 pset.popt.topt.tableAttr = pg_strdup(optarg);
 break;
+			case 'u':
+options->authtype = pg_strdup(optarg);
+break;
 			case 'U':
 options->username = pg_strdup(optarg);
 break;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 4dc8924..b8e77d4 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -304,6 +304,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */
 	offsetof(struct pg_conn, target_session_attrs)},
 
+	{"acc_auth", NULL, NULL, NULL,
+		"Acceptable authentification methods", "", 20,
+	offsetof(struct pg_conn, acc_auth)},
+
 	/* Terminating entry --- MUST BE LAST */
 	{NULL, NULL, NULL, NULL,
 	NULL, NULL, 0}
@@ -1000,6 +1004,43 @@ connectOptions2(PGconn *conn)
 		}
 	}
 
+	/* Validate acceptable authentification methods */
+	if (conn->acc_auth)
+	{
+		char * pvalue = conn->acc_auth;
+		char * comma = pvalue;
+		while ((comma = strchr(pvalue, ',')))
+		{
+			*comma = '\0';
+			if (strcmp(pvalue, "password") != 0
+&& strcmp(pvalue, "md5") != 0
+&& strcmp(pvalue, "scram") != 0
+&& strcmp(pvalue, "gss") != 0
+&& strcmp(pvalue, "sspi") != 0)
+			{
+conn->status = CONNECTION_BAD;
+printfPQExpBuffer(>errorMessage,
+			libpq_gettext("invalid authtype value: \"%s\"\n"),
+			  pvalue);
+return false;
+			}
+			*comma = ',';
+			pvalue = comma + 1;
+		}
+		if (strcmp(pvalue, "password") != 0
+			&& strcmp(pvalue, "md5") != 0
+			&& strcmp(pvalue, "scram") != 0
+			&& strcmp(pvalue, "gss") != 0
+			&& strcmp(pvalue, "sspi") != 0)
+		{
+			conn->status = CONNECTION_BAD;
+			printfPQExpBuffer(>errorMessage,
+		libpq_gettext("invalid authtype value: \"%s\"\n"),
+		  pvalue);
+			return false;
+		}
+	}
+
 	/*
 	 * validate sslmode option
 	 */
@@ -1822,6 +1863,43 @@ restoreErrorMessage(PGconn *conn, PQExpBuffer savedMessage)
 	termPQExpBuffer(savedMessage);
 }
 
+static bool
+isAllowableAuth(int areq, char* acc_auth)
+{
+	/* Allowable authentification scheme is not 

[HACKERS] [GSOC][weekly report 5] Eliminate O(N^2) scaling from rw-conflict tracking in serializable transactions

[Mengxing Liu]

Sorry for report late. Our lab's machines crashed for several days. 


As I reported in the last email,
 
https://www.postgresql.org/message-id/5b6b452.16851.15cf1ec010e.coremail.liu-m...@mails.tsinghua.edu.cn
I tried to decrease the contention on SerializableFinishedListLock. 
It works actually.  But I don't know why my optimization results in another 
problems: more contention on PREDICATELOCK_MANAGER_LWLOCK.


Here is the result of statistics collector. There were 36 connections in all, 
so most of them are waiting for locks. 
 (benchmark is ssibench, but other benchmarks have the same result) 


Original:
SerializableXactHashLock, 0.36
predicate_lock_manager, 6.00
wal_insert, 0.07
SerializableFinishedListLock, 16.50


After optimization: 
SerializableXactHashLock, 0.35
predicate_lock_manager, 11.53
buffer_content, 0.12
SerializableFinishedListLock, 11.47


So in the end, the performance did not change obviously. 
Even if I enlarged the number of predicate locks (NUM_PREDICATELOCK_PARTITIONS) 
from 16 to 1024, 
there were still so many contentions. 


It bothered me for several days. The patch is attached. 
To focus on this issue, I removed other parts of modification. 
Do you have any advices for this ? 



--

Sincerely


Mengxing Liu








finishedlock.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] [PATCH] Minor typo in the source repository documentation

[Tom Lane]

Martin Mai  writes:
> I found a typo while reading the source repository documentation:
> https://www.postgresql.org/docs/devel/static/sourcerepo.html

Hm, I dunno, I like the existing wording better than yours.  I agree
that it's a bit unclear that the antecedent of "they" is meant to be
the tools not the files; but this phraseology seems to make the files
be the active agents, which is just weird.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Minor typo in the source repository documentation

[Martin Mai]

Hello,

I found a typo while reading the source repository documentation:
https://www.postgresql.org/docs/devel/static/sourcerepo.html

Attached patch fixes it.

Cheers
Martin Mai
From 41febee1363aa62babd2b88fbb4ad1a2e8023f13 Mon Sep 17 00:00:00 2001
From: Martin Mai 
Date: Sun, 9 Jul 2017 16:27:45 +0200
Subject: [PATCH] Fix typo in source repository documentation

---
 doc/src/sgml/sourcerepo.sgml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/sourcerepo.sgml b/doc/src/sgml/sourcerepo.sgml
index f8f6bf2de1..dbef5cdd1e 100644
--- a/doc/src/sgml/sourcerepo.sgml
+++ b/doc/src/sgml/sourcerepo.sgml
@@ -20,9 +20,9 @@
Note that building PostgreSQL from the source
repository requires reasonably up-to-date versions of bison,
flex, and Perl. These tools are not needed
-   to build from a distribution tarball since the files they are used to build
-   are included in the tarball.  Other tool requirements are the same as shown
-   in .
+   to build from a distribution tarball since the files that are used to build
+   it are included in the tarball.  Other tool requirements are the same as
+   shown in .
   
 
  
-- 
2.11.0


-- 
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_receivewal and messages printed in non-verbose mode

[Michael Paquier]

On Sun, Jul 9, 2017 at 5:58 AM, Magnus Hagander  wrote:
> I wonder if we should actually just remove the second message? AFAICT no
> other tools log that information. Is there any particular reason why we want
> that logging in pg_receivewal when we don't have it in other tools?

I maintain a fork of pg_receivewal lately that works as a service, and
I have found myself a fan of this log bit when debugging funky issues.
That's a personal opinion, no objections to remove it either.
-- 
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] WIP patch: distinguish selectivity of < from <= and > from >=

[Kuntal Ghosh]

On Fri, Jul 7, 2017 at 2:53 AM, Tom Lane  wrote:
> Kuntal Ghosh  writes:
Wow. Thank you for the wonderful explanation.

>> On Thu, Jul 6, 2017 at 3:45 AM, Tom Lane  wrote:
>
>> if histogram_bounds are assigned as,
>> {0,9,19,29,39,49,59,69,79,89,99,109,119,129,13,..}
>> ...
>> So, if val=low, then hisfrac = (bucket_current - 1)/num_of_buckets
>> which means it assumes val is included in the previous bucket.
>
> I looked at that again and realized that one of the answers I was missing
> lies in the behavior of analyze.c's compute_scalar_stats().  When it has
> collected "nvals" values and wants to make a histogram containing
> "num_hist" entries, it does this:
>
>  * The object of this loop is to copy the first and last values[]
>  * entries along with evenly-spaced values in between.  So the
>  * i'th value is values[(i * (nvals - 1)) / (num_hist - 1)].
>
> (where i runs from 0 to num_hist - 1).  Because the "/" denotes integer
> division, this effectively means that for all but the first entry,
> we are taking the last value out of the corresponding bucket of samples.
> That's obviously true for the last histogram entry, which will be the very
> last sample value, and it's also true for earlier ones --- except for the
> *first* histogram entry, which is necessarily the first one in its bucket.
> So the first histogram bucket is actually a tad narrower than the others,
> which is visible in the typical data you showed above: 0 to 9 is only 9
> counts wide, but all the remaining buckets are 10 counts wide.  That
> explains why we need a scale adjustment just in the first bucket.
>
Agreed. But, I've some doubt regarding the amount of adjustment needed.
+  if (i == 1)
+  histfrac += eq_selec * (1.0 - binfrac);
For predicate x<=p, when p lies in the first bucket, following is the amount
of binfrac that we're off from the actual one.
(Assume the same histogram boundaries i.e., 0,9,19,...)

For p=0, (1/10)-(0/9) = 1/10 * (1 - 0)
For p=1, (2/10)-(1/9) = 1/10 * (1 - 1/9)
For p=2, (3/10)-(2/9) = 1/10 * (1 - 2/9)
For p=3, (4/10)-(3/9) = 1/10 * (1 - 3/9)

In general, 1/(high + 1 - low) * (1.0 - binfrac) and the difference in
histfrac is
(1.0 - binfrac) / (high + 1 - low) / num_of_hist_buckets.

So, the above adjustment for the first bucket looks correct when,
otherdistinct ~ (high + 1 - low) * num_of_hist_buckets

Is that always true or I'm missing something?

> I think I'm also beginning to grasp why scalarineqsel's basic estimation
> process produces an estimate for "x <= p" rather than some other condition
> such as "x < p".  For clarity, imagine that we're given p equal to the
> last histogram entry.  If the test operator is in fact "<=", then it will
> say "true" for every histogram entry, and we'll fall off the end of the
> histogram and return 1.0, which is correct.  If the test operator is "<",
> it will return "true" for all but the last entry, so that we end up with
> "i" equal to sslot.nvalues - 1 --- but we will compute binfrac = 1.0,
> if convert_to_scalar() produces sane answers, again resulting in
> histfrac = 1.0.  Similar reasoning applies for ">" and ">=", so that in
> all cases we arrive at histfrac = 1.0 if p equals the last histogram
> entry.  More generally, if p is equal to some interior histogram entry,
> say the k'th one out of n total, we end up with histfrac = (k-1)/(n-1)
> no matter which operator we probe with, assuming that convert_to_scalar()
> correctly gives us a binfrac of 0.0 or 1.0 depending on which of the
> adjacent bins we end up examining.  So, remembering that the histogram
> entries occupy the right ends of their buckets, the proper interpretation
> of that is that it's the probability of "x <= p".  (This is wrong for the
> first histogram entry, but that's why we need a correction there.)
>
True. In general, histfrac = (k-1)/(n-1) + binfrac where binfrac
depends on the linear interpolation.

For p=some histogram boundary, it'll always be the probability of
"x<=p". When p lies inside the bucket and we've a flat distribution,
then also it'll be the probability of "x<=p". But, when we've high
variance inside a bucket and p lies inside the bucket, linear
interpolation fails to capture the actual probability. But, as you've
mentioned earlier, I guess that's not a new problem.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server

[Michael Paquier]

On Sun, Jul 9, 2017 at 8:00 PM, Stephen Frost  wrote:
> * Noah Misch (n...@leadboat.com) wrote:
>> [Action required within three days.  This is a generic notification.]
>>
>> The above-described topic is currently a PostgreSQL 10 open item.  Stephen,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> v10 open item, please let us know.  Otherwise, please observe the policy on
>> open item ownership[1] and send a status update within three calendar days of
>> this message.  Include a date for your subsequent status update.  Testers may
>> discover new open items at any time, and I want to plan to get them all fixed
>> well in advance of shipping v10.  Consequently, I will appreciate your 
>> efforts
>> toward speedy resolution.  Thanks.
>
> I'm out of town this week but will review the patch from Masahiko and
> provide a status update by July 17th.

Good to know. I was planning to review the patch within 24 hours. I
have spotted at least one bug in the patch: there is no need to wait
for the backup history file and the last segment to be archived on a
standby unless archive_mode = always, meaning that
XLogArchivingAlways() needs to be used instead of
XLogArchivingActive(). There may be other things, but I am lacking
time and brain power now for a complete analysis.
-- 
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_stop_backup(wait_for_archive := true) on standby server

[Stephen Frost]

All,

* Noah Misch (n...@leadboat.com) wrote:
> On Fri, Jun 30, 2017 at 02:59:11PM -0400, Stephen Frost wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> > > On 6/30/17 04:08, Masahiko Sawada wrote:
> > > >> I'm not sure. I think this can be considered a bug in the 
> > > >> implementation for
> > > >> 10, and as such is "open for fixing". However, it's not a very 
> > > >> critical bug
> > > >> so I doubt it should be a release blocker, but if someone wants to 
> > > >> work on a
> > > >> fix I think we should commit it.
> > > > 
> > > > I agree with you. I'd like to hear opinions from other hackers as well.
> > > 
> > > It's preferable to make it work.  If it's not easily possible, then we
> > > should prohibit it.
> > > 
> > > Comments from Stephen (original committer)?
> > 
> > I agree that it'd be preferable to make it work, but I'm not sure I can
> > commit to having it done in short order.  I'm happy to work to prohibit
> > it, but if someone has a few spare cycles to make it actually work,
> > that'd be great.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Stephen,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

I'm out of town this week but will review the patch from Masahiko and
provide a status update by July 17th.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Sun, Jul 9, 2017 at 2:38 AM, Alexander Korotkov 
wrote:

> Could you, please, specify idea of what you're implementing in more
> detail?
>

Ultimatley we would like an indexed scan instead of a sequential scan, so I
thought we needed to index the FK array columns first.

[HACKERS] Re: Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Noah Misch]

On Fri, Jul 07, 2017 at 06:47:26PM +0900, Amit Langote wrote:
> On 2017/07/06 16:06, Etsuro Fujita wrote:
> > Looks odd to me because the error message doesn't show any DETAIL info;
> > since the CTE query, which produces the message, is the same as the above
> > query, the message should also be the same as the one for the above
> > query.
> 
> I agree that the DETAIL should be shown.

> The patch keeps tests that you added in your patch.  Added this to the
> open items list.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] pg_stop_backup(wait_for_archive := true) on standby server

[Noah Misch]

On Fri, Jun 30, 2017 at 02:59:11PM -0400, Stephen Frost wrote:
> Peter, all,
> 
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> > On 6/30/17 04:08, Masahiko Sawada wrote:
> > >> I'm not sure. I think this can be considered a bug in the implementation 
> > >> for
> > >> 10, and as such is "open for fixing". However, it's not a very critical 
> > >> bug
> > >> so I doubt it should be a release blocker, but if someone wants to work 
> > >> on a
> > >> fix I think we should commit it.
> > > 
> > > I agree with you. I'd like to hear opinions from other hackers as well.
> > 
> > It's preferable to make it work.  If it's not easily possible, then we
> > should prohibit it.
> > 
> > Comments from Stephen (original committer)?
> 
> I agree that it'd be preferable to make it work, but I'm not sure I can
> commit to having it done in short order.  I'm happy to work to prohibit
> it, but if someone has a few spare cycles to make it actually work,
> that'd be great.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Stephen,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] Multi column range partition table

[Dean Rasheed]

On 6 July 2017 at 22:43, Joe Conway  wrote:
> I agree we should get this right the first time and I also agree with
> Dean's proposal, so I guess I'm a +2
>

On 7 July 2017 at 03:21, Amit Langote  wrote:
> +1 to releasing this syntax in PG 10.
>

So, that's 3 votes in favour of replacing UNBOUNDED with
MINVALUE/MAXVALUE for range partition bounds in PG 10. Not a huge
consensus, but no objections either. Any one else have an opinion?

Robert, have you been following this thread?

I was thinking of pushing this later today, in time for beta2.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers