date:20170425

On 26/04/17 01:01, Fujii Masao wrote:
> On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
>  wrote:
>> Hello,
>>
>> At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada  
>> wrote in 
> BEGIN;
> ALTER SUBSCRIPTION hoge_sub ENABLE;
> PREPARE TRANSACTION 'g';
> BEGIN;
> SELECT 1;
> COMMIT;  -- wake up the launcher at this point.
> COMMIT PREPARED 'g';
>
> Even if we wake up the launcher even when a ROLLBACK PREPARED, it's
> not a big deal to the launcher process actually. Compared with the
> complexly of changing the logic so that on_commit_launcher_wakeup
> corresponds to prepared transaction, we might want to accept this
> behavior.

 on_commit_launcher_wakeup needs to be recoreded in 2PC state
 file to handle this issue properly. But I agree with you, that would
 be overkill for small gain. So I'm thinking to add the following
 comment into your patch and push it. Thought?

 -
 Note that on_commit_launcher_wakeup flag doesn't work as expected in 2PC 
 case.
 For example, COMMIT PREPARED on the transaction enabling the flag
 doesn't wake up
 the logical replication launcher if ROLLBACK on another transaction
 runs before it.
 To handle this case properly, the flag needs to be recorded in 2PC
 state file so that
 COMMIT PREPARED and ROLLBACK PREPARED can determine whether to wake up
 the launcher. However this is overkill for small gain and false wakeup
 of the launcher
 is not so harmful (probably we can live with that), so we do nothing
 here for this issue.
 -

>>>
>>> Agreed.
>>>
>>> I added this comment to the patch and attached it.
>>
>> The following "However" may need a follwoing comma.
>>
>>> However this is overkill for small gain and false wakeup of the
>>> launcher is not so harmful (probably we can live with that), so
>>> we do nothing here for this issue.
>>
>> I agree this as a whole. But I think that the main issue here is
>> not false wakeups, but 'possible delay of launching new workers
>> by 3 minutes at most' (this is centainly a kind of false wakeups,
>> though). We can live with this failure when using two-paase
>> commmit, but I think it shouldn't happen silently.
>>
>>
>> How about providing AtPrepare_ApplyLauncher(void) like the
>> follows and calling it in PrepareTransaction?
> 
> Or we should apply the attached patch and handle the 2PC case properly?
> I was thinking that it's overkill more than necessary, but that seems not true
> as far as I implement that.
> 

Looks like it does not even increase size of the 2pc file, +1 for this.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

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

Re: [HACKERS] Declarative partitioning - another take

Hi,

Thanks for testing.

On 2017/04/25 19:03, Rajkumar Raghuwanshi wrote:
> Thanks for looking into it. I have applied fixes and checked for triggers.
> I could see difference in behaviour of statement triggers for INSERT and
> UPDATE, for insert only root partition triggers are getting fired but for
> update root as well as child partition table triggers both getting fired.
> is this expected??

Yes, because I didn't implement anything for the insert case yet.  I posed
a question whether to fire partitions' per-statement triggers when
inserting data through the root table.

Robert replied [1] that it would be desirable to not fire partitions'
per-statement triggers if the root table is mentioned in the query; only
fire their per-row triggers if any.  It already works that way for
inserts, and applying only 0001 will get you the same for update/delete.
Patch 0002 is to enable firing partition's per-statement triggers even if
the root table is mentioned in the query, but it implemented the same only
for the update/delete cases.  If we decide that that's the right thing to
do, then I will implement the same behavior for the insert case too.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoatBYy8Hyi3cYR1rFrCkD2NM4ZLZcck4QDGvH%3DHddfDwA%40mail.gmail.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] Declarative partitioning - another take

On 2017/04/26 3:58, Robert Haas wrote:
> On Mon, Apr 24, 2017 at 6:43 AM, Amit Langote
>  wrote:
>> The reason it doesn't work is that we do not allocate ResultRelInfos for
>> partitioned tables (not even for the root partitioned table in the
>> update/delete cases), because the current implementation assumes they are
>> not required.  That's fine only so long as we consider that no rows are
>> inserted into them, no indexes need to be updated, and that no row-level
>> triggers are to be fired.  But it misses the fact that we do allow
>> statement-level triggers on partitioned tables.  So, we should fix things
>> such that ResultRelInfos are allocated so that any statement-level
>> triggers are fired. But there are following questions to consider:
>>
>> 1. Should we consider only the root partitioned table or all partitioned
>>tables in a given hierarchy, including the child partitioned tables?
>>Note that this is related to a current limitation of updating/deleting
>>inheritance hierarchies that we do not currently fire per-statement
>>triggers of the child tables.  See the TODO item in wiki:
>>https://wiki.postgresql.org/wiki/Todo#Triggers, which says: "When
>>statement-level triggers are defined on a parent table, have them fire
>>only on the parent table, and fire child table triggers only where
>>appropriate"
>>
>> 2. Do we apply the same to inserts on the partitioned tables?  Since
>>insert on a partitioned table implicitly affects its partitions, one
>>might say that we would need to fire per-statement insert triggers of
>>all the partitions.
> 
> It seems to me that it doesn't make a lot of sense to fire the
> triggers for some tables involved in the hierarchy and not others.  I
> suppose the consistent thing to do here is to make sure that we fire
> the statement triggers for all tables in the partitioning hierarchy
> for all operations (insert, update, delete, etc.).
> 
> TBH, I don't like that very much.  I'd rather fire the triggers only
> for the table actually named in the query and skip all the others,
> mostly because it seems like it would be faster and less likely to
> block future optimizations; eventually, I'd like to consider not even
> locking the children we're not touching, but that's not going to work
> if we have to check them all for triggers.  So what I'd prefer -- on
> the totally unprincipled basis that it would let us improve
> performance in the future -- if you operate on a partition directly,
> you fire the partition's triggers, but if you operate on the parent,
> only the parent's triggers fire.

FWIW, I too prefer the latter, that is, fire only the parent's triggers.
In that case, applying only the patch 0001 will do.

Thanks,
Amit



-- 
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 10 release notes

On Wed, Apr 26, 2017 at 10:56 AM, Bruce Momjian  wrote:
> First, I don't think RFC references belong in the release notes, let
> alone RFC links.
>
> Second, there seems to be some confusion over what SCRAM-SHA-256 gives
> us over MD5.  I think there are a few benefits:
>
> o  packets cannot be replayed as easily, i.e. md5 replayed random salt
> packets with a 50% probability after 16k sessions
> o  hard to re-use SCRAM-SHA-256 string if disclosed vs. simple for md5
> o  harder to brute-force trying all possible strings to find a matching
> hash
>
> So if you tell users that SCRAM-SHA-256 is better than MD5 only because
> of one of those, they will not realize that three benefits of changing
> to SCRAM-SHA-256.  I might have even missed some benefits.

If the release notes keep a general tone, perhaps it would be better
to mention as well that SCRAM is the recommended password-based
authentication method moving forward?
-- 
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 10 release notes

On Tue, Apr 25, 2017 at 07:17:20PM -0700, Andres Freund wrote:
> On 2017-04-25 22:13:00 -0400, Bruce Momjian wrote:
> > On Tue, Apr 25, 2017 at 06:40:08PM -0700, Andres Freund wrote:
> > > On 2017-04-25 21:19:41 -0400, Bruce Momjian wrote:
> > > > On Tue, Apr 25, 2017 at 06:51:47AM +0200, Petr Jelinek wrote:
> > > > > Or the ability of logical decoding to follow timeline switches.
> > > > 
> > > > When you say "logical decoding", you don't mean contrib/test_decoding?
> > > 
> > > No.  test_decoding is just an example output plugin ("formatting" the
> > > changes), but there's in-production users of logical decoding out
> > > there.  So features adding to logical decoding in general, are worth to
> > > be mention in general.
> > 
> > Yes, but I am still trying to find out what changed because we don't
> > ship much that does "decoding" except contrib/test_decoding.  Are you
> > saying logical information is included in WAL?
> 
> We ship an extension API [1] [2] that allows to use decoding.  And that
> the internals of that API now knows how to deal with timeline changes.
> You could e.g. compare it with the FDW API now providing the ability to
> do aggregate pushdown or such.

Yes, if we changed the developer-visible API we should mention that in
the release notes.

> > > >> I also wonder if ability to run SQL queries on walsender connected to a
> > > >> database is worth mentioning (replication=database kind of connection).
> 
> Yes, but we're still discussing the feature's exact look nearby.

OK, please keep us up-to-date on what needs to be added.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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 10 release notes

On Tue, Apr 25, 2017 at 06:40:08PM -0700, Andres Freund wrote:
> On 2017-04-25 21:19:41 -0400, Bruce Momjian wrote:
> > On Tue, Apr 25, 2017 at 06:51:47AM +0200, Petr Jelinek wrote:
> > > Or the ability of logical decoding to follow timeline switches.
> > 
> > When you say "logical decoding", you don't mean contrib/test_decoding?
> 
> No.  test_decoding is just an example output plugin ("formatting" the
> changes), but there's in-production users of logical decoding out
> there.  So features adding to logical decoding in general, are worth to
> be mention in general.

Yes, but I am still trying to find out what changed because we don't
ship much that does "decoding" except contrib/test_decoding.  Are you
saying logical information is included in WAL?

> > Do you mean the WAL logical stream now has timeline information and 3rd
> > party software should know about that?
> 
> It's less about knowing about it, and more about being empowering new 
> usecases.
> 
> But, TBH, in this case I'm not sure what the point would be - given
> there's no way to sanely create and use such slots, I don't think the
> timeline following has advantages on its own, it's imo more of a
> stepping stone.

OK, makes sense.  Let me know if we should add:

> >> I also wonder if ability to run SQL queries on walsender connected to a
> >> database is worth mentioning (replication=database kind of connection).

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

On Wed, Apr 26, 2017 at 4:53 AM, Simon Riggs  wrote:
> On 25 April 2017 at 16:28, Tom Lane  wrote:
>> Simon Riggs  writes:
>>> I can't see any reason now why overwriteOK should exist at all. I'm
>>> guessing that the whole "overwriteOK" idea was an incorrect response
>>> to xids appearing where they shouldn't have done because of the
>>> mistake you just corrected. So I will now remove the parameter from
>>> the call.
>>
>> Seems reasonable, but I don't like the logic change you made in
>> SubTransSetParent; you broke the former invariant, for non-Assert
>> builds, that the target pg_subtrans entry is guaranteed to have
>> the correct value on exit.  I do like fixing it to not dirty the
>> page unnecessarily, but I'd suggest that we write it like
>>
>> if (*ptr != parent)
>> {
>> Assert(*ptr == InvalidTransactionId);
>> *ptr = parent;
>> SubTransCtl->shared->page_dirty[slotno] = true;
>> }
>
> OK, thanks. I'll commit that tomorrow.

A couple of comments about the last patch of this thread.

Nit: there is a complain about a trailing whitespace:
$ git diff --check
src/backend/access/transam/twophase.c:2011: trailing whitespace.
+ bool fromdisk,

+* It's possible that SubTransSetParent has been set before, if
+* the prepared transaction generated xid assignment records.
 */
for (i = 0; i < hdr->nsubxacts; i++)
-   SubTransSetParent(subxids[i], xid, true);
+   SubTransSetParent(subxids[i], xid);
You can remove this part in RecoverPreparedTransactions() and just
switch ProcessTwoPhaseBuffer()'s setParent to true to do the same
thing. The existing comment block should be moved before
ProcessTwoPhaseBuffer() call. It is critical to mention that
pg_subtrans is not kept after a restart.
-- 
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 10 release notes

On 2017-04-25 22:13:00 -0400, Bruce Momjian wrote:
> On Tue, Apr 25, 2017 at 06:40:08PM -0700, Andres Freund wrote:
> > On 2017-04-25 21:19:41 -0400, Bruce Momjian wrote:
> > > On Tue, Apr 25, 2017 at 06:51:47AM +0200, Petr Jelinek wrote:
> > > > Or the ability of logical decoding to follow timeline switches.
> > > 
> > > When you say "logical decoding", you don't mean contrib/test_decoding?
> > 
> > No.  test_decoding is just an example output plugin ("formatting" the
> > changes), but there's in-production users of logical decoding out
> > there.  So features adding to logical decoding in general, are worth to
> > be mention in general.
> 
> Yes, but I am still trying to find out what changed because we don't
> ship much that does "decoding" except contrib/test_decoding.  Are you
> saying logical information is included in WAL?

We ship an extension API [1] [2] that allows to use decoding.  And that
the internals of that API now knows how to deal with timeline changes.
You could e.g. compare it with the FDW API now providing the ability to
do aggregate pushdown or such.


> > >> I also wonder if ability to run SQL queries on walsender connected to a
> > >> database is worth mentioning (replication=database kind of connection).

Yes, but we're still discussing the feature's exact look nearby.

Greetings,

Andres Freund

[1] https://www.postgresql.org/docs/devel/static/logicaldecoding.html
[2] 
https://www.postgresql.org/docs/devel/static/logicaldecoding-output-plugin.html


-- 
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 10 release notes

On 2017-04-25 21:19:41 -0400, Bruce Momjian wrote:
> On Tue, Apr 25, 2017 at 06:51:47AM +0200, Petr Jelinek wrote:
> > Or the ability of logical decoding to follow timeline switches.
> 
> When you say "logical decoding", you don't mean contrib/test_decoding?

No.  test_decoding is just an example output plugin ("formatting" the
changes), but there's in-production users of logical decoding out
there.  So features adding to logical decoding in general, are worth to
be mention in general.

> Do you mean the WAL logical stream now has timeline information and 3rd
> party software should know about that?

It's less about knowing about it, and more about being empowering new usecases.

But, TBH, in this case I'm not sure what the point would be - given
there's no way to sanely create and use such slots, I don't think the
timeline following has advantages on its own, it's imo more of a
stepping stone.

Greetings,

Andres Freund

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

Re: [HACKERS] PG 10 release notes

On Wed, Apr 26, 2017 at 11:06:03AM +0900, Michael Paquier wrote:
> On Wed, Apr 26, 2017 at 10:56 AM, Bruce Momjian  wrote:
> > First, I don't think RFC references belong in the release notes, let
> > alone RFC links.
> >
> > Second, there seems to be some confusion over what SCRAM-SHA-256 gives
> > us over MD5.  I think there are a few benefits:
> >
> > o  packets cannot be replayed as easily, i.e. md5 replayed random salt
> > packets with a 50% probability after 16k sessions
> > o  hard to re-use SCRAM-SHA-256 string if disclosed vs. simple for md5
> > o  harder to brute-force trying all possible strings to find a matching
> > hash
> >
> > So if you tell users that SCRAM-SHA-256 is better than MD5 only because
> > of one of those, they will not realize that three benefits of changing
> > to SCRAM-SHA-256.  I might have even missed some benefits.
> 
> If the release notes keep a general tone, perhaps it would be better
> to mention as well that SCRAM is the recommended password-based
> authentication method moving forward?

Well, we could add "MD5 users are encouraged to switch to
SCRAM-SHA-256".  Now whether we want to list this as something on the
SCRAM-SHA-256 description, or mention it as an incompatibility, or
under Migration.  I am not clear that MD5 is in such terrible shape that
this is warranted.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Dropping a partitioned table takes too long

Hi,

On 2017/04/25 20:07, 高增琦 wrote:
> 
> 2017-04-25 15:07 GMT+08:00 Amit Langote :
> 
>> $SUBJECT, if the table has, say, 2000 partitions.
>>
>> The main reason seems to be that RelationBuildPartitionDesc() will be
>> called that many times within the same transaction, which perhaps we
>> cannot do much about right away.  But one thing we could do is to reduce
>> the impact of memory allocations it does.  They are currently leaked into
>> the caller's context, which may not be reset immediately (such as
>> PortalHeapMemory).  Instead of doing it in the caller's context, use a
>> temporary context that is deleted before returning.  Attached is a patch
>> for that.  On my local development VM, `drop table
>> table_with_2000_partitions` finished in 27 seconds with the patch instead
>> of more than 20 minutes that it currently takes.
>
> The attached patch try to replace 'heap_open' with 'LockRelationOid' when
> locking parent table.
> It improved dropping a table with 7000 partitions.

Your patch seems to be a much better solution to the problem, thanks.

Regards,
Amit



-- 
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] Dropping a partitioned table takes too long

On 2017/04/25 20:55, Ashutosh Bapat wrote:
> On Tue, Apr 25, 2017 at 12:37 PM, Amit Langote
>  wrote:
>> $SUBJECT, if the table has, say, 2000 partitions.
>>
>> The main reason seems to be that RelationBuildPartitionDesc() will be
>> called that many times within the same transaction, which perhaps we
>> cannot do much about right away.  But one thing we could do is to reduce
>> the impact of memory allocations it does.  They are currently leaked into
>> the caller's context, which may not be reset immediately (such as
>> PortalHeapMemory).  Instead of doing it in the caller's context, use a
>> temporary context that is deleted before returning.  Attached is a patch
>> for that.  On my local development VM, `drop table
>> table_with_2000_partitions` finished in 27 seconds with the patch instead
>> of more than 20 minutes that it currently takes.
>>
>> Thoughts?
>>
> I am not able to undestand why does changing memory context cause so
> much difference in execution time?
> 
> The way this patch uses the memory context in this patch, it's
> possible that in future we will allocate something in temporary
> context and then refer it from a longer context. Instead, may be we
> should free things specially or change memory context only when
> allocating those things.

Actually, I am withdrawing this patch for time being, because a much
direct and better solution has been offered upthread by GaoZengqi.
Anyway, temporary context added by my patch would not contain any objects
that will be accessed outside of RelationBuildPartitionDesc().  Remember
that the content that we put into the longer-lived rd_partdesc uses the
memory allocated in rd_pdcxt, not the temporary context that I proposed.

Thanks,
Amit



-- 
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 10 release notes

On Wed, Apr 26, 2017 at 09:02:51AM +0900, Michael Paquier wrote:
> On Wed, Apr 26, 2017 at 12:20 AM, Bruce Momjian  wrote:
> > On Tue, Apr 25, 2017 at 02:39:40PM +0900, Michael Paquier wrote:
> >> 
> >> Add SCRAM-SHA-256
> >> support for password negotiation and storage (Michael
> >> Paquier, Heikki Linnakangas)
> >> 
> >> 
> >> This proves better security than the existing 'md5' negotiation and
> >> storage method.
> >> 
> >> This is quite vague...
> >
> > Can you give me better text?  I can't think of any.
> 
> Sure, here is an idea:
> Add support for SASL authentication using protocol mechanism
> SCRAM-SHA-256 per RFC 5802 and 7677. (adding a reference to the RFCs
> with a link seems important to me).
> 
> SCRAM-SHA-256 improves deficiencies of MD5 password hashing by
> preventing any kind of pass-the-hash vulnerabilities, where a user
> would be able to connect to a PostgreSQL instance by just knowing the
> hash of a password and not the password itself.

First, I don't think RFC references belong in the release notes, let
alone RFC links.

Second, there seems to be some confusion over what SCRAM-SHA-256 gives
us over MD5.  I think there are a few benefits:

o  packets cannot be replayed as easily, i.e. md5 replayed random salt
packets with a 50% probability after 16k sessions

o  hard to re-use SCRAM-SHA-256 string if disclosed vs. simple for md5

o  harder to brute-force trying all possible strings to find a matching
hash

So if you tell users that SCRAM-SHA-256 is better than MD5 only because
of one of those, they will not realize that three benefits of changing
to SCRAM-SHA-256.  I might have even missed some benefits.
 
-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] logical replication and PANIC during shutdown checkpoint in publisher

On Wed, Apr 26, 2017 at 3:17 AM, Peter Eisentraut
 wrote:
> On 4/21/17 00:11, Michael Paquier wrote:
>> Hmm. I have been actually looking at this solution and I am having
>> doubts regarding its robustness. In short this would need to be
>> roughly a two-step process:
>> - In PostmasterStateMachine(), SIGUSR2 is sent to the checkpoint to
>> make it call ShutdownXLOG(). Prior doing that, a first signal should
>> be sent to all the WAL senders with
>> SignalSomeChildren(BACKEND_TYPE_WALSND). SIGUSR2 or SIGINT could be
>> used.
>> - At reception of this signal, all WAL senders switch to a stopping
>> state, refusing commands that can generate WAL.
>> - Checkpointer looks at the state of all WAL senders, looping with a
>> sleep call of a couple of ms, refusing to launch the shutdown
>> checkpoint as long as all WAL senders have not switched to the
>> stopping state.
>> - In reaper(), once checkpointer is confirmed as stopped, signal again
>> the WAL senders, and tell them to perform the last loop.
>
> Yeah that looks like a reasonable approach.
>
> I'm not sure why in your patch you process got_SIGUSR2 in
> WalSndErrorCleanup() instead of in the main loop.

Yes I was hesitating about this one when hacking it. Thinking an extra
time, the similar check in StartReplication() should also not use
got_SIGUSR2 to give the WAL sender a chance to do more work while the
shutdown checkpoint is running as it could take minutes.

Attached is an updated patch to reflect that.
-- 
Michael


walsender-chkpt-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] PG 10 release notes

On Tue, Apr 25, 2017 at 06:51:47AM +0200, Petr Jelinek wrote:
> I also wonder if ability to run SQL queries on walsender connected to a
> database is worth mentioning (replication=database kind of connection).
> 
> Or the ability of logical decoding to follow timeline switches.

When you say "logical decoding", you don't mean contrib/test_decoding? 
Do you mean the WAL logical stream now has timeline information and 3rd
party software should know about that?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Re: logical replication and PANIC during shutdown checkpoint in publisher

On Wed, Apr 26, 2017 at 4:26 AM, Peter Eisentraut
 wrote:
> On 4/20/17 11:30, Peter Eisentraut wrote:
>> We have a possible solution but need to work out a patch.  Let's say
>> next check-in on Monday.
>
> Update: We have a patch that looks promising, but we haven't made much
> progress in reviewing the details.  I'll work on it this week, and
> perhaps Michael also has time to work on it this week.

Next week is Golden Week in Japan so I'll have limited access to an
electronic devices. This week should be fine.

> We could use more eyes.  Next check-in Friday.

Reviews always welcome.
-- 
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 10 release notes

2017-04-25 Thread Tsunakawa, Takayuki

From: 'Bruce Momjian' [mailto:br...@momjian.us]
> > I forgot to point out one thing.
> >
> > Allow libpq to connect to multiple specified host names (Robert Haas)
> > libpq will connect with the first responsive host name.
> >
> > According to the following CF entry and my memory,
> >
> > https://commitfest.postgresql.org/12/879/
> >
> > Authors
> > mithun cy (mithun.cy)
> >
> > Reviewers
> > Gerdan Santos (gerdan), Takayuki Tsunakawa (maumau)Remove from
> > reviewers
> >
> > Committer
> > Robert Haas (rhaas)
> >
> > The author should probably be "(Mithun Cy, Robert Haas, Victor Wagner)"
> 
> I based the release note text on this commit:
> 
>   commit 274bb2b3857cc987cfa21d14775cae9b0dababa5
>   Author: Robert Haas 
>   Date:   Thu Nov 3 09:25:20 2016 -0400
> 
>   libpq: Allow connection strings and URIs to specify multiple
> hosts.
> 
>   It's also possible to specify a separate port for each host.
> 
>   Previously, we'd loop over every address returned by looking
> up the
>   host name; now, we'll try every address for every host name.
> 
>   Patch by me.  Victor Wagner wrote an earlier patch for this
> feature,
>   which I read, but I didn't use any of his code.  Review by Mithun
> Cy.
> 
> Was it wrong?

Oh, that commit.  I got it.

Regards
Takayuki Tsunakawa



-- 
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 10 release notes

2017-04-25 Thread 'Bruce Momjian'

On Wed, Apr 26, 2017 at 12:26:33AM +, Tsunakawa, Takayuki wrote:
> Hello, Bruce
> 
> I forgot to point out one thing.
> 
> Allow libpq to connect to multiple specified host names (Robert Haas) 
> libpq will connect with the first responsive host name. 
> 
> According to the following CF entry and my memory,
> 
> https://commitfest.postgresql.org/12/879/
> 
> Authors
> mithun cy (mithun.cy) 
> 
> Reviewers
> Gerdan Santos (gerdan), Takayuki Tsunakawa (maumau)Remove from reviewers 
> 
> Committer
> Robert Haas (rhaas)  
> 
> The author should probably be "(Mithun Cy, Robert Haas, Victor Wagner)"

I based the release note text on this commit:

commit 274bb2b3857cc987cfa21d14775cae9b0dababa5
Author: Robert Haas 
Date:   Thu Nov 3 09:25:20 2016 -0400

libpq: Allow connection strings and URIs to specify multiple hosts.

It's also possible to specify a separate port for each host.

Previously, we'd loop over every address returned by looking up the
host name; now, we'll try every address for every host name.

Patch by me.  Victor Wagner wrote an earlier patch for this feature,
which I read, but I didn't use any of his code.  Review by Mithun 
Cy.

Was it wrong?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

-- 
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] Cached plans and statement generalization

2017-04-25 Thread Tsunakawa, Takayuki

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Konstantin
> Knizhnik
> Well, first of all I want to share results I already get: pgbench with default
> parameters,  scale 10 and one connection:
> 
> So autoprepare is as efficient as explicit prepare and can increase
> performance almost two times.

This sounds great.

BTW, when are the autoprepared statements destroyed?  Are you considering some 
upper limit on the number of prepared statements?

Regards
Takayuki Tsunakawa




-- 
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] Separation walsender & normal backends

On 2017-04-26 08:41:46 +0800, Craig Ringer wrote:
> I'd very much like to reduce the amount of magic global juggling done
> by the walsender, unify the XLogRead paths, unify the timeline
> following logic for physical walsenders, logical walsenders and
> logical decoding on normal backends, allow normal backends to be
> signaled when there's new WAL, etc. I think there's a fair bit to do
> in order to do this well though.

That all seems orthogonal however.

- Andres


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

Re: [HACKERS] scram and \password

On Wed, Apr 26, 2017 at 12:26 AM, Heikki Linnakangas  wrote:
> Yeah, there is that. But we simply cannot change the signature of an
> existing function. It would not only produce compile-time errors when
> building old applications, which would arguably be a good thing, but it
> would also cause old binaries to segfault when dynamically linked with the
> new libpq.

Sure.

> I think it's clear that we should have a new function that takes the
> algorithm as argument. But there are open decisions on what the old
> PQencryptPassword() function should do, and also what the new function
> should do by default, if you don't specify an algorithm:
>
> A) Have PQencryptPassword() return an md5 hash.
>
> B) Have PQencryptPassword() return a SCRAM verifier
>
> C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10
> server, and an md5 hash otherwise. This is tricky, because PQencryptPassword
> doesn't take a PGconn argument. It could behave like PQescapeString() does,
> and choose md5/scram depending on the server version of the last connection
> that was established.

Like the rest I vote for A, and document it as deprecated.

> 
> char *
> PQencryptPasswordConn(PGconn *conn,
>   const char *passwd,
>   const char *user,
>   const char *method,
>   const char *options)
>
> [...]

Good for me, I was first thinking as well about having "default" as
keyword, NULL is fine as well.

Could it be possible to name the new function as PQhashPassword
instead of PQencryptPassword? From the previous threads we agreed that
encryption makes no sense in this context.
-- 
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] Separation walsender & normal backends

2017-04-25 Thread Craig Ringer

On 26 April 2017 at 02:36, Andres Freund  wrote:

> For
> logical rep we'd alternatively add more complexity because we'd need
> both replication and non-replication connections (to stream changes, to
> copy tables, to query config), which'd also complicate administration
> because users & hba config have to be setup so the same user can connect
> over both.

We have experience of this from BDR and pglogical, of course, and it's
definitely a pain and source of user misconfiguration.

I'm not sure how practical it is to merge walsenders and regular
backends at this stage of pg10 development, but it seems like a
worthwhile goal down the track. I'd very much like to reduce the
amount of magic global juggling done by the walsender, unify the
XLogRead paths, unify the timeline following logic for physical
walsenders, logical walsenders and logical decoding on normal
backends, allow normal backends to be signaled when there's new WAL,
etc. I think there's a fair bit to do in order to do this well though.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

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

Re: [HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

2017-04-25 Thread Craig Ringer

On 26 April 2017 at 08:30, Huong Dangminh  wrote:

> Default for hot_standby parameter should be "on" from PostgreSQL 10?
>
> In PostgreSQL 10, -w option is default for [pg_ctl start].
> So in order to start standby we have to setting hot_standby to "on" or
> start standby with -W option.

Yeah. That's a good reason to change it. I think at this point warm
standby is clearly the less-used secondary option and hot_standby
should be default.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

2017-04-25 Thread Huong Dangminh

Hi,

Default for hot_standby parameter should be "on" from PostgreSQL 10?

In PostgreSQL 10, -w option is default for [pg_ctl start]. 
So in order to start standby we have to setting hot_standby to "on" or 
start standby with -W option. 

Change hot_standby to "on" will fix this inconvenience.
wal_level is also changed default to "replica" in PostgreSQL 10, 
this might not be affect to primary.

Just a little bit but I attached the patch.
---
Thanks and best regards, 
Dang Minh Huong 
NEC Solution Innovators, Ltd. 
http://www.nec-solutioninnovators.co.jp/en/



hot_standby.patch
Description: hot_standby.patch

-- 
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 10 release notes

2017-04-25 Thread Tsunakawa, Takayuki

Hello, Bruce

I forgot to point out one thing.

Allow libpq to connect to multiple specified host names (Robert Haas) 
libpq will connect with the first responsive host name. 

According to the following CF entry and my memory,

https://commitfest.postgresql.org/12/879/

Authors
mithun cy (mithun.cy) 

Reviewers
Gerdan Santos (gerdan), Takayuki Tsunakawa (maumau)Remove from reviewers 

Committer
Robert Haas (rhaas)  

The author should probably be "(Mithun Cy, Robert Haas, Victor Wagner)"

Regards
Takayuki Tsunakawa





-- 
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 10 release notes

On Wed, Apr 26, 2017 at 12:20 AM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 02:39:40PM +0900, Michael Paquier wrote:
>> 
>> Add SCRAM-SHA-256
>> support for password negotiation and storage (Michael
>> Paquier, Heikki Linnakangas)
>> 
>> 
>> This proves better security than the existing 'md5' negotiation and
>> storage method.
>> 
>> This is quite vague...
>
> Can you give me better text?  I can't think of any.

Sure, here is an idea:
Add support for SASL authentication using protocol mechanism
SCRAM-SHA-256 per RFC 5802 and 7677. (adding a reference to the RFCs
with a link seems important to me).

SCRAM-SHA-256 improves deficiencies of MD5 password hashing by
preventing any kind of pass-the-hash vulnerabilities, where a user
would be able to connect to a PostgreSQL instance by just knowing the
hash of a password and not the password itself.
-- 
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] question: data file update when pg_basebackup in progress

On Wed, Apr 26, 2017 at 1:45 AM, David G. Johnston
 wrote:
> The first write to a page after a checkpoint is always recorded in the WAL
> as a full page write.  Every WAL file since the checkpoint must also be
> copied to the backed up system.  The replay of those WAL files is what
> brings the remote and local system into sync with respect to all changes
> since the backup checkpoint.

Bringing to the point that the presence of backup_label in a backup is
critical, as this tells Postgres from which position in WAL it should
begin recovery to bring the system up to a consistent state.
pg_basebackup also makes sure that the last WAL segment needed is
archived before the backup completes so as recovery can completely be
done.
-- 
Michael


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

[HACKERS] Transition tables for triggers on foreign tables and views

2017-04-25 Thread Thomas Munro

Hi hackers,

My colleague Prabhat Sahu reported off list that transition tables
don't work for views.  I probably should have thought about that when
I fixed something similar for partitioned tables, and after some
experimentation I see that this is also broken for foreign tables.

For foreign tables using postgres_fdw, I see that transition tables
capture new rows for INSERT but capture nothing for DELETE and UPDATE.

For views, aside from the question of transition tables, I noticed
that statement triggers don't seem to fire at all with updatable
views.  Surely they should -- isn't that a separate bug?

Example:

  create table my_table (i int);
  create view my_view as select * from my_table;
  create function my_trigger_function() returns trigger language plpgsql as
$$ begin raise warning 'hello world'; return null; end; $$;
  create trigger my_trigger after insert or update or delete on my_view
for each statement execute procedure my_trigger_function();
  insert into my_view values (42);

... and the world remains ungreeted.

As for transition tables, there are probably meaningful ways to
support those for both views and foreign tables at least in certain
cases, as future feature enhancements.  For now, do you agree that we
should reject such triggers as unsupported?  See attached.

Separately, I noticed an obsolete sentence in the trigger
documentation.  See doc.patch.

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


prevent-unsupported-transition-tables.patch
Description: Binary data


doc.patch
Description: Binary data

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

Re: [HACKERS] Cached plans and statement generalization

via Newton Mail 
[https://cloudmagic.com/k/d/mailapp?ct=dx=9.4.52=10.11.6=email_footer_2]
On Tue, Apr 25, 2017 at 3:48 PM, Doug Doole  wrote: It's 
not always that simple, at least in postgres, unless you disregard
search_path. Consider e.g. cases like

CREATE SCHEMA a;
CREATE SCHEMA b;
CREATE TABLE a.foobar(somecol int);
SET search_patch = 'b,a';
SELECT * FROM foobar;
CREATE TABLE b.foobar(anothercol int);
SELECT * FROM foobar; -- may not be cached plan from before!

it sounds - my memory of DB2 is very faint, and I never used it much -
like similar issues could arise in DB2 too?

DB2 does handle this case. Unfortunately I don't know the details of how it 
worked though.
A naive option would be to invalidate anything that depends on table or view 
*.FOOBAR. You could probably make it a bit smarter by also requiring that 
schema A appear in the path. While this specific scenario does not arise in DB2 
since it uses CURRENT SCHEMA only for tables (much to my dislike) your examples 
holds for functions and types which are resolved by path. For encapsulated SQL 
(in views, functions) conservative semantics are enforced via including the 
timestamp. For dynamic SQL the problem you describe does exist though and I 
think it is handled in the way Doug describes. However, as noted by Doug the 
topic of plan invalidation is really orthogonal to normalizing the queries. All 
it does is provide more opportunities to run into any pre-existing bugs.
Cheers Serge
PS: I’m just starting to look at the plan invalidation code in PG because we 
are dealing with potentially 10s of thousands of cached SQL statements. So 
these complete wipe outs or walks of every plan in the cache don’t scale.

Re: [HACKERS] some review comments on logical rep code

2017-04-25 Thread Fujii Masao

On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada  
> wrote in 
>> >> BEGIN;
>> >> ALTER SUBSCRIPTION hoge_sub ENABLE;
>> >> PREPARE TRANSACTION 'g';
>> >> BEGIN;
>> >> SELECT 1;
>> >> COMMIT;  -- wake up the launcher at this point.
>> >> COMMIT PREPARED 'g';
>> >>
>> >> Even if we wake up the launcher even when a ROLLBACK PREPARED, it's
>> >> not a big deal to the launcher process actually. Compared with the
>> >> complexly of changing the logic so that on_commit_launcher_wakeup
>> >> corresponds to prepared transaction, we might want to accept this
>> >> behavior.
>> >
>> > on_commit_launcher_wakeup needs to be recoreded in 2PC state
>> > file to handle this issue properly. But I agree with you, that would
>> > be overkill for small gain. So I'm thinking to add the following
>> > comment into your patch and push it. Thought?
>> >
>> > -
>> > Note that on_commit_launcher_wakeup flag doesn't work as expected in 2PC 
>> > case.
>> > For example, COMMIT PREPARED on the transaction enabling the flag
>> > doesn't wake up
>> > the logical replication launcher if ROLLBACK on another transaction
>> > runs before it.
>> > To handle this case properly, the flag needs to be recorded in 2PC
>> > state file so that
>> > COMMIT PREPARED and ROLLBACK PREPARED can determine whether to wake up
>> > the launcher. However this is overkill for small gain and false wakeup
>> > of the launcher
>> > is not so harmful (probably we can live with that), so we do nothing
>> > here for this issue.
>> > -
>> >
>>
>> Agreed.
>>
>> I added this comment to the patch and attached it.
>
> The following "However" may need a follwoing comma.
>
>> However this is overkill for small gain and false wakeup of the
>> launcher is not so harmful (probably we can live with that), so
>> we do nothing here for this issue.
>
> I agree this as a whole. But I think that the main issue here is
> not false wakeups, but 'possible delay of launching new workers
> by 3 minutes at most' (this is centainly a kind of false wakeups,
> though). We can live with this failure when using two-paase
> commmit, but I think it shouldn't happen silently.
>
>
> How about providing AtPrepare_ApplyLauncher(void) like the
> follows and calling it in PrepareTransaction?

Or we should apply the attached patch and handle the 2PC case properly?
I was thinking that it's overkill more than necessary, but that seems not true
as far as I implement that.

Regards,

-- 
Fujii Masao


002_Reset_on_commit_launcher_wakeup_v5.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] Cached plans and statement generalization

2017-04-25 Thread Finnerty, Jim


On 4/25/17, 6:34 PM, "pgsql-hackers-ow...@postgresql.org on behalf of Andres 
Freund"  
wrote:

It's not always that simple, at least in postgres, unless you disregard
search_path.  Consider e.g. cases like

CREATE SCHEMA a;
CREATE SCHEMA b;
CREATE TABLE a.foobar(somecol int);
SET search_patch = 'b,a';
SELECT * FROM foobar;
CREATE TABLE b.foobar(anothercol int);
SELECT * FROM foobar; -- may not be cached plan from before!

it sounds - my memory of DB2 is very faint, and I never used it much -
like similar issues could arise in DB2 too?

Greetings,

Andres Freund


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


You may need to store the reloid of each relation in the cached query to ensure 
that the schema bindings are the same, and also invalidate dependent cache 
entries if a referenced relation is dropped. 

Regards,

Jim Finnerty


-- 
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] Cached plans and statement generalization

2017-04-25 Thread Doug Doole

>
> (FWIW, on this list we don't do top-quotes)
>

I know. Forgot and just did "reply all". My bad.

It's not always that simple, at least in postgres, unless you disregard
> search_path.  Consider e.g. cases like
>
> CREATE SCHEMA a;
> CREATE SCHEMA b;
> CREATE TABLE a.foobar(somecol int);
> SET search_patch = 'b,a';
> SELECT * FROM foobar;
> CREATE TABLE b.foobar(anothercol int);
> SELECT * FROM foobar; -- may not be cached plan from before!
>
> it sounds - my memory of DB2 is very faint, and I never used it much -
> like similar issues could arise in DB2 too?
>

DB2 does handle this case. Unfortunately I don't know the details of how it
worked though.

A naive option would be to invalidate anything that depends on table or
view *.FOOBAR. You could probably make it a bit smarter by also requiring
that schema A appear in the path.

- Doug

Re: [HACKERS] Cached plans and statement generalization

2017-04-25 Thread David G. Johnston

On Tue, Apr 25, 2017 at 3:24 PM, David Fetter  wrote:

> I don't have an exploit yet.  What concerns me is attackers' access to
> what is in essence the ability to poke at RULEs when they only have
> privileges to read.
>

If they want to see how it works they can read the source code.  In terms
of runtime data it would limited to whatever the session itself created.
In most cases the presence of the cache would be invisible.  I suppose it
might appear if one were to explain a query, reset the session, explain
another query and then re-explain the original.  If the chosen plan in the
second pass differed because of the presence of the leading query it would
be noticeable but not revealing.  Albeit I'm a far cry from a security
expert...

David J.

Re: [HACKERS] Cached plans and statement generalization

Hi,

(FWIW, on this list we don't do top-quotes)

On 2017-04-25 22:21:22 +, Doug Doole wrote:
> Plan invalidation was no different than for any SQL statement. DB2 keeps a
> list of the objects the statement depends on. If any of the objects changes
> in an incompatible way the plan is invalidated and kicked out of the cache.
> 
> I suspect what is more interesting is plan lookup. DB2 has something called
> the "compilation environment". This is a collection of everything that
> impacts how a statement is compiled (SQL path, optimization level, etc.).
> Plan lookup is done using both the statement text and the compilation
> environment. So, for example, if my path is DOUG, MYTEAM, SYSIBM and your
> path is ANDRES, MYTEAM, SYSIBM we will have different compilation
> environments. If we both issue "SELECT * FROM T" we'll end up with
> different cache entries even if T in both of our statements resolves to
> MYTEAM.T. If I execute "SELECT * FROM T", change my SQL path and then
> execute "SELECT * FROM T" again, I have a new compilation environment so
> the second invocation of the statement will create a new entry in the
> cache. The first entry is not kicked out - it will still be there for
> re-use if I change my SQL path back to my original value (modulo LRU for
> cache memory management of course).

It's not always that simple, at least in postgres, unless you disregard
search_path.  Consider e.g. cases like

CREATE SCHEMA a;
CREATE SCHEMA b;
CREATE TABLE a.foobar(somecol int);
SET search_patch = 'b,a';
SELECT * FROM foobar;
CREATE TABLE b.foobar(anothercol int);
SELECT * FROM foobar; -- may not be cached plan from before!

it sounds - my memory of DB2 is very faint, and I never used it much -
like similar issues could arise in DB2 too?

Greetings,

Andres Freund


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

Re: [HACKERS] Cached plans and statement generalization

2017-04-25 Thread David Fetter

On Tue, Apr 25, 2017 at 11:35:21PM +0300, Konstantin Knizhnik wrote:
> On 04/25/2017 07:54 PM, David Fetter wrote:
> > On Tue, Apr 25, 2017 at 06:11:09PM +0300, Konstantin Knizhnik wrote:
> > > On 24.04.2017 21:43, Andres Freund wrote:
> > > > Hi,
> > > > 
> > > > On 2017-04-24 11:46:02 +0300, Konstantin Knizhnik wrote:
> > > > > So what I am thinking now is implicit query caching. If the same 
> > > > > query with
> > > > > different literal values is repeated many times, then we can try to
> > > > > generalize this query and replace it with prepared query with
> > > > > parameters.
> > > > That's not actuall all that easy:
> > > > - You pretty much do parse analysis to be able to do an accurate match.
> > > > How much overhead is parse analysis vs. planning in your cases?
> > > > - The invalidation infrastructure for this, if not tied to to fully
> > > > parse-analyzed statements, is going to be hell.
> > > > - Migrating to parameters can actually cause significant slowdowns, not
> > > > nice if that happens implicitly.
> > > Well, first of all I want to share results I already get: pgbench with
> > > default parameters,  scale 10 and one connection:
> > > 
> > > protocol
> > >   TPS
> > > simple
> > >   3492
> > > extended
> > >   2927
> > > prepared
> > >   6865
> > > simple + autoprepare
> > >   6844
> > If this is string mashing on the unparsed query, as it appears to be,
> > it's going to be a perennial source of security issues.
> 
> Sorry, may be I missed something, but I can not understand how
> security can be violated by extracting string literals from query. I
> am just copying bytes from one buffer to another. I do not try to
> somehow interpret this parameters.  What I am doing is very similar
> with standard prepared statements.  And moreover query is parsed!
> Only query which was already parsed and executed (but with different
> values of parameters) can be autoprepared.

I don't have an exploit yet.  What concerns me is attackers' access to
what is in essence the ability to poke at RULEs when they only have
privileges to read.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Cached plans and statement generalization

2017-04-25 Thread Doug Doole

Plan invalidation was no different than for any SQL statement. DB2 keeps a
list of the objects the statement depends on. If any of the objects changes
in an incompatible way the plan is invalidated and kicked out of the cache.

I suspect what is more interesting is plan lookup. DB2 has something called
the "compilation environment". This is a collection of everything that
impacts how a statement is compiled (SQL path, optimization level, etc.).
Plan lookup is done using both the statement text and the compilation
environment. So, for example, if my path is DOUG, MYTEAM, SYSIBM and your
path is ANDRES, MYTEAM, SYSIBM we will have different compilation
environments. If we both issue "SELECT * FROM T" we'll end up with
different cache entries even if T in both of our statements resolves to
MYTEAM.T. If I execute "SELECT * FROM T", change my SQL path and then
execute "SELECT * FROM T" again, I have a new compilation environment so
the second invocation of the statement will create a new entry in the
cache. The first entry is not kicked out - it will still be there for
re-use if I change my SQL path back to my original value (modulo LRU for
cache memory management of course).

With literal replacement, the cache entry is on the modified statement
text. Given the modified statement text and the compilation environment,
you're guaranteed to get the right plan entry.

On Tue, Apr 25, 2017 at 2:47 PM Andres Freund  wrote:

> On 2017-04-25 21:11:08 +, Doug Doole wrote:
> > When I did this in DB2, I didn't use the parser - it was too expensive. I
> > just tokenized the statement and used some simple rules to bypass the
> > invalid cases. For example, if I saw the tokens "ORDER" and "BY" then I'd
> > disallow replacement replacement until I hit the end of the current
> > subquery or statement.
>
> How did you manage plan invalidation and such?
>
> - Andres
>

Re: [HACKERS] Separation walsender & normal backends

On 2017-04-25 23:24:40 +0200, Petr Jelinek wrote:
> On 25/04/17 17:13, Fujii Masao wrote:
> > On Tue, Apr 25, 2017 at 11:34 PM, Tom Lane  wrote:
> >> Andres Freund  writes:
> >>> I've for a while suspected that the separation & duplication of
> >>> infrastructure between walsenders and normal backends isn't nice.
> >>
> >> I think we should consider a more radical solution: trying to put
> >> general SQL query capability into the replication protocol was a
> >> bad idea and we should revert it while we still can.  The uglinesses
> >> you mention aren't merely implementation issues, they're an indication
> >> that that concept is broken in itself.
> > 
> > I think that it's worth considering this option in order to "stabilize"
> > logical replication stuff before the release. The table sync patch
> > (which allows walsender to run normal queries) introduced such
> > uglinesses and increased the complexity in logical rep code.
> > OTOH, I believe that logical replication is still useful even without
> > initial table sync feature. So reverting the table sync patch seems
> > possible idea.
> > 
> 
> I don't think that's good idea, the usefulness if much lower without the
> initial copy.

Agreed. I think that'd move us way backwards, and we'd have to tackle
exactly the same issue in a few weeks again.

> The original patch for this added new commands to
> replication protocol, adding generic SQL interface was result of request
> in the reviews.

Yea, I still think it's the right approach in general - I don't think
the patch itself was properly discussed and such though, being
essentially burried in another commit.


> I personally don't mind moving back my original idea of special commands
> if that was the consensus, but previous consensus was to do SQL instead.

I really don't think that'll solve anything.

- Andres


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

Re: [HACKERS] Cached plans and statement generalization

On 2017-04-25 21:11:08 +, Doug Doole wrote:
> When I did this in DB2, I didn't use the parser - it was too expensive. I
> just tokenized the statement and used some simple rules to bypass the
> invalid cases. For example, if I saw the tokens "ORDER" and "BY" then I'd
> disallow replacement replacement until I hit the end of the current
> subquery or statement.

How did you manage plan invalidation and such?

- Andres


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

Re: [HACKERS] Separation walsender & normal backends

2017-04-25 Thread David G. Johnston

On Tue, Apr 25, 2017 at 2:24 PM, Petr Jelinek 
wrote:

> On 25/04/17 17:13, Fujii Masao wrote:
> > On Tue, Apr 25, 2017 at 11:34 PM, Tom Lane  wrote:
> > OTOH, I believe that logical replication is still useful even without
> > initial table sync feature. So reverting the table sync patch seems
> > possible idea.
> >
>
> I don't think that's good idea, the usefulness if much lower without the
> initial copy. The original patch for this added new commands to
> replication protocol, adding generic SQL interface was result of request
> in the reviews.
>

Haven't followed this feature closely but my first thoughts when recently
reading about it were related to the initial copy and table synchronization
- so I'd have to agree with Petr here.  Full table sync is big and for any
table with activity on it the confidence level of knowing you have
everything is greatly reduced if the system isn't making a guarantee.

David J.

Re: [HACKERS] Separation walsender & normal backends

On 25/04/17 17:13, Fujii Masao wrote:
> On Tue, Apr 25, 2017 at 11:34 PM, Tom Lane  wrote:
>> Andres Freund  writes:
>>> I've for a while suspected that the separation & duplication of
>>> infrastructure between walsenders and normal backends isn't nice.
>>
>> I think we should consider a more radical solution: trying to put
>> general SQL query capability into the replication protocol was a
>> bad idea and we should revert it while we still can.  The uglinesses
>> you mention aren't merely implementation issues, they're an indication
>> that that concept is broken in itself.
> 
> I think that it's worth considering this option in order to "stabilize"
> logical replication stuff before the release. The table sync patch
> (which allows walsender to run normal queries) introduced such
> uglinesses and increased the complexity in logical rep code.
> OTOH, I believe that logical replication is still useful even without
> initial table sync feature. So reverting the table sync patch seems
> possible idea.
> 

I don't think that's good idea, the usefulness if much lower without the
initial copy. The original patch for this added new commands to
replication protocol, adding generic SQL interface was result of request
in the reviews.

I personally don't mind moving back my original idea of special commands
if that was the consensus, but previous consensus was to do SQL instead.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Cached plans and statement generalization

2017-04-25 Thread Doug Doole

When I did this in DB2, I didn't use the parser - it was too expensive. I
just tokenized the statement and used some simple rules to bypass the
invalid cases. For example, if I saw the tokens "ORDER" and "BY" then I'd
disallow replacement replacement until I hit the end of the current
subquery or statement.

There are a few limitations to this approach. For example, DB2 allowed you
to cast using function notation like VARCHAR(foo, 10). This meant I would
never replace the second parameter of any VARCHAR function. Now it's
possible that when the statement was fully compiled we'd find that
VARCHAR(foo,10) actually resolved to BOB.VARCHAR() instead of the built-in
cast function. Our thinking that these cases were rare enough that we
wouldn't worry about them. (Of course, PostgreSQL's ::VARCHAR(10) syntax
avoids this problem completely.)

Because SQL is so structured, the implementation ended up being quite
simple (a few hundred line of code) with no significant maintenance issues.
(Other developers had no problem adding in new cases where constants had to
be preserved.)

The simple tokenizer was also fairly extensible. I'd prototyped using the
same code to also normalize statements (uppercase all keywords, collapse
whitespace to a single blank, etc.) but that feature was never added to the
product.

- Doug

On Tue, Apr 25, 2017 at 1:47 PM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

> On 04/25/2017 11:40 PM, Serge Rielau wrote:
>
>
> On Apr 25, 2017, at 1:37 PM, Konstantin Knizhnik <
> k.knizh...@postgrespro.ru> wrote:
>
>
> SELECT ‘hello’::CHAR(10) || ‘World’, 5 + 6;
>
> You can substitute ‘hello’, ‘World’, 5, and 6. But not 10.
>
>
> I am substituting only string literals. So the query above will be
> transformed to
>
> SELECT $1::CHAR(10) || $2, 5 + 6;
>
> What's wrong with it?
>
>
> Oh, well that leaves a lot of opportunities on the table, doesn’t it?
>
>
> Well, actually my primary intention was not to make badly designed
> programs (not using prepared statements) work faster.
> I wanted to address cases when it is not possible to use prepared
> statements.
> If we want to substitute with parameters as much literals as possible,
> then parse+deparse tree seems to be the only reasonable approach.
> I will try to implement it also, just to estimate parsing overhead.
>
>
>
>
> Cheers
> Serge
>
>
>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

On 2017-04-25 21:22:44 +0100, Simon Riggs wrote:
> On 24 April 2017 at 19:59, Andres Freund  wrote:
> 
> > I don't think that's generally true.
> 
> If you think that, from a risk perspective it is enough for me to
> continue to investigate and I have been doing that.

I'm not sure it's worth digging it all that much - I think we just
shouldn't claim in the release notes that the bug doesn't have any
consequences, but that it's though to only matter in unlikely corner
cases.

- Andres

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

Re: [HACKERS] Cached plans and statement generalization


On 04/25/2017 11:40 PM, Serge Rielau wrote:



On Apr 25, 2017, at 1:37 PM, Konstantin Knizhnik > wrote:



SELECT ‘hello’::CHAR(10) || ‘World’, 5 + 6;

You can substitute ‘hello’, ‘World’, 5, and 6. But not 10.


I am substituting only string literals. So the query above will be transformed 
to

SELECT $1::CHAR(10) || $2, 5 + 6;

What's wrong with it?


Oh, well that leaves a lot of opportunities on the table, doesn’t it?


Well, actually my primary intention was not to make badly designed programs 
(not using prepared statements) work faster.
I wanted to address cases when it is not possible to use prepared statements.
If we want to substitute with parameters as much literals as possible, then 
parse+deparse tree seems to be the only reasonable approach.
I will try to implement it also, just to estimate parsing overhead.





Cheers
Serge




--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Cached plans and statement generalization


> On Apr 25, 2017, at 1:37 PM, Konstantin Knizhnik  
> wrote:
>> 
>> SELECT ‘hello’::CHAR(10) || ‘World’, 5 + 6;
>> 
>> You can substitute ‘hello’, ‘World’, 5, and 6. But not 10.
> 
> I am substituting only string literals. So the query above will be 
> transformed to 
> 
> SELECT $1::CHAR(10) || $2, 5 + 6;
> 
> What's wrong with it?

Oh, well that leaves a lot of opportunities on the table, doesn’t it?

Cheers
Serge

Re: [HACKERS] Cached plans and statement generalization


On 04/25/2017 08:09 PM, Serge Rielau wrote:


On Tue, Apr 25, 2017 at 9:45 AM, Konstantin Knizhnik 
 wrote:

On 25.04.2017 19:12, Serge Rielau wrote:


On Apr 25, 2017, at 8:11 AM, Konstantin Knizhnik > wrote:
Another problem is caused by using integer literals in context where 
parameters can not be used, for example "order by 1”.

You will also need to deal with modifiers in types such as VARCHAR(10). 
Not sure if there are specific functions which can only deal with literals (?) 
as well.

Sorry, I do not completely understand how presence of type modifiers can 
affect string literals used in query.
Can you provide me some example?

SELECT ‘hello’::CHAR(10) || ‘World’, 5 + 6;

You can substitute ‘hello’, ‘World’, 5, and 6. But not 10.


I am substituting only string literals. So the query above will be transformed 
to

SELECT $1::CHAR(10) || $2, 5 + 6;

What's wrong with it?



Also some OLAP syntax like “rows preceding”

It pretty much boils down to whether you can do some shallow parsing rather 
than expending the effort to build the parse tree.

Cheers
Serge



--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Cached plans and statement generalization

On 04/25/2017 07:54 PM, David Fetter wrote:

On Tue, Apr 25, 2017 at 06:11:09PM +0300, Konstantin Knizhnik wrote:

On 24.04.2017 21:43, Andres Freund wrote:

Hi,

On 2017-04-24 11:46:02 +0300, Konstantin Knizhnik wrote:

So what I am thinking now is implicit query caching. If the same query with
different literal values is repeated many times, then we can try to
generalize this query and replace it with prepared query with
parameters.

That's not actuall all that easy:
- You pretty much do parse analysis to be able to do an accurate match.
How much overhead is parse analysis vs. planning in your cases?
- The invalidation infrastructure for this, if not tied to to fully
parse-analyzed statements, is going to be hell.
- Migrating to parameters can actually cause significant slowdowns, not
nice if that happens implicitly.

Well, first of all I want to share results I already get: pgbench with
default parameters, scale 10 and one connection:

protocol
TPS
simple
3492
extended
2927
prepared
6865
simple + autoprepare
6844

If this is string mashing on the unparsed query, as it appears to be,
it's going to be a perennial source of security issues.

Sorry, may be I missed something, but I can not understand how security can be violated by extracting string literals from query. I am just copying bytes from one buffer to another. I do not try to somehow interpret this parameters. What I am doing is
very similar with standard prepared statements.

And moreover query is parsed! Only query which was already parsed and executed
(but with different values of parameters) can be autoprepared.

Best,
David.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

2017-04-25 Thread Simon Riggs

On 24 April 2017 at 19:59, Andres Freund  wrote:

> I don't think that's generally true.

If you think that, from a risk perspective it is enough for me to
continue to investigate and I have been doing that.

As I said before I thought I had found a problem.

I'm suggesting we take the approach that if there is a problem we can
recreate it as a way of exploring what conditions are required and
therefore work out the impact. Nikhil Sontakke appears to have
re-created something, but not quite what I had expected. I think he
will post here tomorrow with an update for us to discuss.

-- 
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] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

2017-04-25 Thread Simon Riggs

On 25 April 2017 at 16:28, Tom Lane  wrote:
> Simon Riggs  writes:
>> I can't see any reason now why overwriteOK should exist at all. I'm
>> guessing that the whole "overwriteOK" idea was an incorrect response
>> to xids appearing where they shouldn't have done because of the
>> mistake you just corrected. So I will now remove the parameter from
>> the call.
>
> Seems reasonable, but I don't like the logic change you made in
> SubTransSetParent; you broke the former invariant, for non-Assert
> builds, that the target pg_subtrans entry is guaranteed to have
> the correct value on exit.  I do like fixing it to not dirty the
> page unnecessarily, but I'd suggest that we write it like
>
> if (*ptr != parent)
> {
> Assert(*ptr == InvalidTransactionId);
> *ptr = parent;
> SubTransCtl->shared->page_dirty[slotno] = true;
> }

OK, thanks. I'll commit that tomorrow.

-- 
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] scram and \password

2017-04-25 Thread Magnus Hagander

On Tue, Apr 25, 2017 at 8:29 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas 
> wrote:
> >> A) Have PQencryptPassword() return an md5 hash.
> >>
> >> B) Have PQencryptPassword() return a SCRAM verifier
> >>
> >> C) Have PQencryptPassword() return a SCRAM verifier if connected to a
> v10
> >> server, and an md5 hash otherwise. This is tricky, because
> PQencryptPassword
> >> doesn't take a PGconn argument. It could behave like PQescapeString()
> does,
> >> and choose md5/scram depending on the server version of the last
> connection
> >> that was established.
>
> > I vote for A - leave PQencryptPassword() as-is, and deprecate it.
> > Tell people to use the new function going forward.
>
> +1.  I never much liked that magic behavior of PQescapeString, and don't
> think we should replicate it elsewhere, so I definitely don't like (C).
> And I don't think we can do (B) because that will break the functionality
> altogether when talking to an older server.  That leaves (A) plus invent
> a new function.
>

+1.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/

Re: [HACKERS] tablesync patch broke the assumption that logical rep depends on?

On 22/04/17 22:09, Petr Jelinek wrote:
> On 21/04/17 16:31, Petr Jelinek wrote:
>> On 21/04/17 16:23, Peter Eisentraut wrote:
>>> On 4/21/17 10:11, Petr Jelinek wrote:
 On 21/04/17 16:09, Peter Eisentraut wrote:
> On 4/20/17 14:29, Petr Jelinek wrote:
>> +/* Find unused worker slot. */
>> +if (!w->in_use)
>>  {
>> -worker = >workers[slot];
>> -break;
>> +worker = w;
>> +slot = i;
>> +}
>
> Doesn't this still need a break?  Otherwise it always picks the last slot.
>

 Yes it will pick the last slot, does that matter though, is the first
 one better somehow?

 We can't break because we also need to continue the counter (I think the
 issue that the counter solves is probably just theoretical, but still).
>>>
>>> I see.  I think the code would be less confusing if we break the loop
>>> like before and call logicalrep_sync_worker_count() separately.
>>>
 Hmm actually, maybe the if (!w->in_use) should be if (worker == NULL &&
 !w->in_use)?
>>>
>>> That would also do it.  But it's getting a bit fiddly.
>>>
>>
>> I just wanted to avoid looping twice, especially since the garbage
>> collecting code has to also do the loop. I guess I'll go with my
>> original coding for this then which was to put retry label above the
>> loop first, then try finding worker slot, if found call the
>> logicalrep_sync_worker_count and if not found do the garbage collection
>> and if we cleaned up something then goto retry.
>>
> 
> Here is the patch doing just that.
> 

And one more revision which also checks in_use when attaching shared
memory. This is mainly to improve the user visible behavior in
theoretical corner case when the worker is supposed to be cleaned up but
actually still manages to start (it would still fail even without this
change but the error message would be more obscure).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Fix-various-concurrency-issues-in-logical-replicatiov4.patch
Description: binary/octet-stream

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

Re: [HACKERS] PG 10 release notes

On Tue, Apr 25, 2017 at 03:17:29PM -0400, Tels wrote:
> > I think all that was missing was "time":
> >
> > By default planning and execution time is display by
> > EXPLAIN ANALYZE and not display in other cases.
> > The new EXPLAIN option SUMMARY allows
> > explicit control of this.
> 
> Hm, no, I disagree, the the grammar is not right:
> 
> It should be: "X and Y _are_ display_ed_"
> 
> instead of:
> 
> "X and Y is display"
> 
> likewise "and not display_ed_ in other cases."

OK, got it:

By default planning and execution time are display by
EXPLAIN ANALYZE and are not display in other cases.
The new EXPLAIN option SUMMARY allows
explicit control of this.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Interval for launching the table sync worker

On 4/20/17 15:36, Peter Eisentraut wrote:
> On 4/19/17 23:02, Noah Misch wrote:
>> This PostgreSQL 10 open item is past due for your status update.  Kindly send
>> a status update within 24 hours, and include a date for your subsequent 
>> status
>> update.  Refer to the policy on open item ownership:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> This is currently on the back burner relative to other issues.  Next
> check-in from me by Monday.

Update: I think there are some light disagreements about whether to go
for a simple localized patch or a more extensive rework.  If the latter
camp doesn't convince me, I'll commit the former solution by Friday.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] PG 10 release notes

On Tue, Apr 25, 2017 at 08:12:05PM +0200, Petr Jelinek wrote:
> On 25/04/17 17:01, Bruce Momjian wrote:
> > 
> >> I also wonder if ability to run SQL queries on walsender connected to a
> >> database is worth mentioning (replication=database kind of connection).
> > 
> > Uh, why would that be important to users?
> 
> Because every tool that uses logical decoding to capture changes does not
> need to open extra connection to fetch initial state.
> 
> (and we document much less user visible changes than this in the release
> notes)

Are you saying that 3rd party applications that use logical WAL records
need to know about that?  Can you provide some text?

> >> Or the ability of logical decoding to follow timeline switches.
> > 
> > I didn't think logical decoding was really more than a proof-of-concept
> > until now.
> > 
> 
> Huh?

Uh, the only logical decoding code that I know we ship pre-PG 10 is
contrib/test_decoding/.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Re: logical replication and PANIC during shutdown checkpoint in publisher

On 4/20/17 11:30, Peter Eisentraut wrote:
> On 4/19/17 23:04, Noah Misch wrote:
>> This PostgreSQL 10 open item is past due for your status update.  Kindly send
>> a status update within 24 hours, and include a date for your subsequent 
>> status
>> update.  Refer to the policy on open item ownership:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> We have a possible solution but need to work out a patch.  Let's say
> next check-in on Monday.

Update: We have a patch that looks promising, but we haven't made much
progress in reviewing the details.  I'll work on it this week, and
perhaps Michael also has time to work on it this week.  We could use
more eyes.  Next check-in Friday.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Tels


On Tue, April 25, 2017 1:21 pm, Bruce Momjian wrote:
> On Tue, Apr 25, 2017 at 01:06:05PM -0400, Tels wrote:
>> Moin,
>>
>> On Mon, April 24, 2017 9:31 pm, Bruce Momjian wrote:
>> > I have committed the first draft of the Postgres 10 release notes.
>> They
>> > are current as of two days ago, and I will keep them current.  Please
>> > give me any feedback you have.
>>
>> Thank you! Here is one thing I noticed:
>>
>> "By default planning and execution is display by EXPLAIN ANALYZE and not
>> display in other cases. The new EXPLAIN option SUMMARY allows explicit
>> control of this."
>>
>> The grammar sounds a bit off, I'd suggest:
>>
>> "By default planning and execution time are both displayed by EXPLAIN
>> ANALYZE and not displayed in other cases. The new EXPLAIN option SUMMARY
>> allows explicit control of this feature."
>
> I think all that was missing was "time":
>
> By default planning and execution time is display by
> EXPLAIN ANALYZE and not display in other cases.
> The new EXPLAIN option SUMMARY allows
> explicit control of this.

Hm, no, I disagree, the the grammar is not right:

It should be: "X and Y _are_ display_ed_"

instead of:

"X and Y is display"

likewise "and not display_ed_ in other cases."

Best wishes,

Tels


-- 
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] Declarative partitioning - another take

On Mon, Apr 24, 2017 at 6:43 AM, Amit Langote
 wrote:
> The reason it doesn't work is that we do not allocate ResultRelInfos for
> partitioned tables (not even for the root partitioned table in the
> update/delete cases), because the current implementation assumes they are
> not required.  That's fine only so long as we consider that no rows are
> inserted into them, no indexes need to be updated, and that no row-level
> triggers are to be fired.  But it misses the fact that we do allow
> statement-level triggers on partitioned tables.  So, we should fix things
> such that ResultRelInfos are allocated so that any statement-level
> triggers are fired. But there are following questions to consider:
>
> 1. Should we consider only the root partitioned table or all partitioned
>tables in a given hierarchy, including the child partitioned tables?
>Note that this is related to a current limitation of updating/deleting
>inheritance hierarchies that we do not currently fire per-statement
>triggers of the child tables.  See the TODO item in wiki:
>https://wiki.postgresql.org/wiki/Todo#Triggers, which says: "When
>statement-level triggers are defined on a parent table, have them fire
>only on the parent table, and fire child table triggers only where
>appropriate"
>
> 2. Do we apply the same to inserts on the partitioned tables?  Since
>insert on a partitioned table implicitly affects its partitions, one
>might say that we would need to fire per-statement insert triggers of
>all the partitions.

It seems to me that it doesn't make a lot of sense to fire the
triggers for some tables involved in the hierarchy and not others.  I
suppose the consistent thing to do here is to make sure that we fire
the statement triggers for all tables in the partitioning hierarchy
for all operations (insert, update, delete, etc.).

TBH, I don't like that very much.  I'd rather fire the triggers only
for the table actually named in the query and skip all the others,
mostly because it seems like it would be faster and less likely to
block future optimizations; eventually, I'd like to consider not even
locking the children we're not touching, but that's not going to work
if we have to check them all for triggers.  So what I'd prefer -- on
the totally unprincipled basis that it would let us improve
performance in the future -- if you operate on a partition directly,
you fire the partition's triggers, but if you operate on the parent,
only the parent's triggers fire.

How heretical is that idea?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Claudio Freire

On Tue, Apr 25, 2017 at 2:45 PM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 10:37:48AM -0700, Andres Freund wrote:
>> On 2017-04-25 13:11:32 -0400, Bruce Momjian wrote:
>> > I don't think this warrants inclusion in the release notes for reasons
>> > already discussed.  The vacuum truncation operation is a rare one and
>> > an implementation detail.
>>
>> I think that's backwards. The truncate operation is quite delicate
>> because it happens with AccessExclusiveLock held.  This regularly does
>> cause issues in production.  When users look for things they possibly
>> should update for, something like "performance improvents in final
>> vacuum phase" + oneliner is going to be a lot more interesting than,
>> say, "Add MONEY operators for multiplication and division with INT8
>> values".
>>
>> More and more users are going to be primarily interested in three
>> classes of new things in postgres: 1) performance 2) replication 3)
>> easier management. Arbitrarily excluding one of the major categories
>> from release notes isn't a useful policy, especially if we continue to
>> list new feature that'll effect no more than a handful of people.
>
> I disputed that my logic is arbitrary.  However, given your
> explanation, I have added the item:
>
>Improve speed of VACUUM's removal of trailing empty
>heap pages (Alvaro Herrera)

That's enough for me, thanks.


-- 
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 doesn't start immediately on eabled

On 4/6/17 08:24, Kyotaro HORIGUCHI wrote:
> The attached patch wakes up launcher when a subscription is
> enabled. This fails when a subscription is enabled immedaitely
> after disabling but it won't be a matter.

committed, thanks

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Separation walsender & normal backends

On 2017-04-25 10:34:20 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I've for a while suspected that the separation & duplication of
> > infrastructure between walsenders and normal backends isn't nice.
> 
> I think we should consider a more radical solution: trying to put
> general SQL query capability into the replication protocol was a
> bad idea and we should revert it while we still can.  The uglinesses
> you mention aren't merely implementation issues, they're an indication
> that that concept is broken in itself.

I don't think that's the right solution, I think it's evidence that the
split was a bad idea in the first place.  There's a growing amount of
duplication between the protocols, and a growing number of use-cases
that need facilities of both protocols.  We e.g. now already have SHOW
statements in both, except that they behave slightly differently.  For
logical rep we'd alternatively add more complexity because we'd need
both replication and non-replication connections (to stream changes, to
copy tables, to query config), which'd also complicate administration
because users & hba config have to be setup so the same user can connect
over both.

Therefore I think it's the implementation that's not perfect, but the
idea is perfectly sound.  Having two awkwardly & increasingly different
languages in postgres doesn't sound like a sound idea.

- Andres

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

Re: [HACKERS] scram and \password

2017-04-25 Thread Tom Lane

Robert Haas  writes:
> On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas  wrote:
>> A) Have PQencryptPassword() return an md5 hash.
>> 
>> B) Have PQencryptPassword() return a SCRAM verifier
>> 
>> C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10
>> server, and an md5 hash otherwise. This is tricky, because PQencryptPassword
>> doesn't take a PGconn argument. It could behave like PQescapeString() does,
>> and choose md5/scram depending on the server version of the last connection
>> that was established.

> I vote for A - leave PQencryptPassword() as-is, and deprecate it.
> Tell people to use the new function going forward.

+1.  I never much liked that magic behavior of PQescapeString, and don't
think we should replicate it elsewhere, so I definitely don't like (C).
And I don't think we can do (B) because that will break the functionality
altogether when talking to an older server.  That leaves (A) plus invent
a new function.

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] logical replication and PANIC during shutdown checkpoint in publisher

On 4/21/17 00:11, Michael Paquier wrote:
> Hmm. I have been actually looking at this solution and I am having
> doubts regarding its robustness. In short this would need to be
> roughly a two-step process:
> - In PostmasterStateMachine(), SIGUSR2 is sent to the checkpoint to
> make it call ShutdownXLOG(). Prior doing that, a first signal should
> be sent to all the WAL senders with
> SignalSomeChildren(BACKEND_TYPE_WALSND). SIGUSR2 or SIGINT could be
> used.
> - At reception of this signal, all WAL senders switch to a stopping
> state, refusing commands that can generate WAL.
> - Checkpointer looks at the state of all WAL senders, looping with a
> sleep call of a couple of ms, refusing to launch the shutdown
> checkpoint as long as all WAL senders have not switched to the
> stopping state.
> - In reaper(), once checkpointer is confirmed as stopped, signal again
> the WAL senders, and tell them to perform the last loop.

Yeah that looks like a reasonable approach.

I'm not sure why in your patch you process got_SIGUSR2 in
WalSndErrorCleanup() instead of in the main loop.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] scram and \password

On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas  wrote:
> algorithm as argument. But there are open decisions on what the old
> PQencryptPassword() function should do, and also what the new function
> should do by default, if you don't specify an algorithm:
>
> A) Have PQencryptPassword() return an md5 hash.
>
> B) Have PQencryptPassword() return a SCRAM verifier
>
> C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10
> server, and an md5 hash otherwise. This is tricky, because PQencryptPassword
> doesn't take a PGconn argument. It could behave like PQescapeString() does,
> and choose md5/scram depending on the server version of the last connection
> that was established.

I vote for A - leave PQencryptPassword() as-is, and deprecate it.
Tell people to use the new function going forward.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] PG 10 release notes

On 25/04/17 17:01, Bruce Momjian wrote:
> 
>> I also wonder if ability to run SQL queries on walsender connected to a
>> database is worth mentioning (replication=database kind of connection).
> 
> Uh, why would that be important to users?

Because every tool that uses logical decoding to capture changes does no
need to open extra connection to fetch initial state.

(and we document much less user visible changes than this in the release
notes)

> 
>> Or the ability of logical decoding to follow timeline switches.
> 
> I didn't think logical decoding was really more than a proof-of-concept
> until now.
> 

Huh?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] PG 10 release notes

Hi,

On 2017-04-25 13:39:07 -0400, Bruce Momjian wrote:
> Understood, but the question is whether the release notes are the right
> place to educate users of something that will no longer be a problem.

I think it's the *prime* place for it.  It obviously doesn't matter if
you're not affected by $performance_problem, but if you are affected,
then it's quite likely going to be important.  Deciding whether to
migrate to a new version will often be a decision about a *lot* of work,
so it'll not be made lightly, and without a motivator.

If we go by that logic, why are we listing parallelism? Why are we
listing new planner/executor features that lead to faster plans?  The
reason we do, is that they're addressing concerns that users had, which
they need to know about.

> I am happy to adjust things to whatever the community wants, but, on the
> other hand I have a responsibility to be consistent what what they have
> wanted in the past.

Why?

Greetings,

Andres Freund

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

Re: [HACKERS] Interval for launching the table sync worker

I feel it's getting a bit late for reworkings of this extent, also
considering the marginal nature of the problem we are trying to fix.  My
patch from April 18 is very localized and gets the job done.

I think this is still a good direction to investigate, but if we have to
extend the hash table API to get it done, this might not be the best time.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Adding support for Default partition in partitioning

On Tue, Apr 25, 2017 at 1:20 AM, Ashutosh Bapat
 wrote:
>> I suspect it could be done as of now, but I'm a little worried that it
>> might create grammar conflicts in the future as we extend the syntax
>> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
>> word DEFAULT appears in the same position where we'd normally have FOR
>> VALUES, and so the parser will definitely be able to figure out what's
>> going on.  When it gets to that position, it will see FOR or it will
>> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
>> DEFAULT PARTITION OF ..., then we have action at a distance: whether
>> or not the word DEFAULT is present before PARTITION affects which
>> tokens are legal after the parent table name.
>
> As long as we handle this at the transformation stage, it shouldn't be
> a problem. The grammar would be something like
> CREATE TABLE ... optDefault PARTITION OF ...
>
> If user specifies DEFAULT PARTITION OF t1 FOR VALUES ..., parser will
> allow that but in transformation stage, we will detect it and throw an
> error "DEFAULT partitions can not contains partition bound clause" or
> something like that. Also, documentation would say that DEFAULT and
> partition bound specification are not allowed together.

That's not what I'm concerned about.  I'm concerned about future
syntax additions resulting in difficult-to-resolve grammar conflicts.
For an example what of what I mean, consider this example:

http://postgr.es/m/9253.1295031...@sss.pgh.pa.us

The whole thread is worth a read.  In brief, I wanted to add syntax
like LOCK VIEW xyz, and it wasn't possible to do that without breaking
backward compatibility.  In a nutshell, the problem with making that
syntax work was that LOCK VIEW NOWAIT would then potentially mean
either lock a table called VIEW with the NOWAIT option, or else it
might mean lock a view called NOWAIT.  If the NOWAIT key word were not
allowed at the end or if the TABLE keyword were mandatory, then it
would be possible to make it work, but because we already decided both
to make the TABLE keyword optional and allow an optional NOWAIT
keyword at the end, the syntax couldn't be further extended in the way
that I wanted to extend it without confusing the parser.  The problem
was basically unfixable without breaking backward compatibility, and
we gave up.  I don't want to make the same mistake with the default
partition syntax that we made with the LOCK TABLE syntax.

Aside from unfixable grammar conflicts, there's another way that this
kind of syntax can become problematic, which is when you end up with
multiple optional keywords in the same part of the syntax.  For an
example of that, see
http://postgr.es/m/603c8f070905231747j2e099c23hef8eafbf26682...@mail.gmail.com
- that discusses the problems with EXPLAIN; we later ran into the same
problem with VACUUM.  Users can't remember whether they are supposed
to type VACUUM FULL VERBOSE or VACUUM VERBOSE FULL and trying to
support both creates parser problems and tends to involve adding too
many keywords, so we switched to a new and more extensible syntax for
future options.

Now, you may think that that's never going to happen in this case.
What optional keyword other than DEFAULT could we possibly want to add
just before PARTITION OF?  TBH, I don't know.  I can't think of
anything else we might want to put in that position right now.  But
considering that it's been less than six months since the original
syntax was committed and we've already thought of ONE thing we might
want to put there, it seems hard to rule out the possibility that we
might eventually think of more, and then we will have exactly the same
kind of problem that we've had in the past with other commands.  Let's
head the problem off at the pass and pick a syntax which isn't
vulnerable to that sort of issue.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Re: [HACKERS] PG 10 release notes

On Tue, Apr 25, 2017 at 01:44:09PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Tue, Apr 25, 2017 at 10:00:52AM -0700, Andres Freund wrote:
> >> at this point, you can see, we've squarely left O(N) country, and
> >> entered the vast O(N^2) waste.
> 
> > OK, I got it now. :-)  Here is the new item:
> 
> > Improve table creation speed in sessions that reference many
> > relations (Aleksander Alekseev)
> 
> That's not specifically about table creation.  How about
> 
>   Reduce statistics tracking overhead in sessions that reference
>   many thousands of relations (Aleksander Alekseev)
> 
> I think it is fair, based on Andres' example, to quantify this as not
> being of much interest till you get to thousands of relations.  But
> it would matter for any action that creates a statistics report,
> whether it be scans, inserts, updates, deletes ...

OK, text updated.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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 10 release notes

On Tue, Apr 25, 2017 at 10:37:48AM -0700, Andres Freund wrote:
> On 2017-04-25 13:11:32 -0400, Bruce Momjian wrote:
> > I don't think this warrants inclusion in the release notes for reasons
> > already discussed.  The vacuum truncation operation is a rare one and
> > an implementation detail.
> 
> I think that's backwards. The truncate operation is quite delicate
> because it happens with AccessExclusiveLock held.  This regularly does
> cause issues in production.  When users look for things they possibly
> should update for, something like "performance improvents in final
> vacuum phase" + oneliner is going to be a lot more interesting than,
> say, "Add MONEY operators for multiplication and division with INT8
> values".
> 
> More and more users are going to be primarily interested in three
> classes of new things in postgres: 1) performance 2) replication 3)
> easier management. Arbitrarily excluding one of the major categories
> from release notes isn't a useful policy, especially if we continue to
> list new feature that'll effect no more than a handful of people.

I disputed that my logic is arbitrary.  However, given your
explanation, I have added the item:

   Improve speed of VACUUM's removal of trailing empty
   heap pages (Alvaro Herrera)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Interval for launching the table sync worker

On 4/21/17 09:59, Petr Jelinek wrote:
> Rereading the code again, it's actually not bug as we update the rstate
> to what syncworker says, but it's obviously confusing so probably still
> worth to commit that.

We don't have the syncworker->relmutex at that point, so it's probably
better to read the previously-copied value in rstate->state.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Tom Lane

Bruce Momjian  writes:
> On Tue, Apr 25, 2017 at 10:00:52AM -0700, Andres Freund wrote:
>> at this point, you can see, we've squarely left O(N) country, and
>> entered the vast O(N^2) waste.

> OK, I got it now. :-)  Here is the new item:

> Improve table creation speed in sessions that reference many
> relations (Aleksander Alekseev)

That's not specifically about table creation.  How about

Reduce statistics tracking overhead in sessions that reference
many thousands of relations (Aleksander Alekseev)

I think it is fair, based on Andres' example, to quantify this as not
being of much interest till you get to thousands of relations.  But
it would matter for any action that creates a statistics report,
whether it be scans, inserts, updates, deletes ...

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] PG 10 release notes

On Tue, Apr 25, 2017 at 02:31:50PM -0300, Claudio Freire wrote:
> >> Author: Álvaro Herrera, loosely based on a submission by Claudio Freire
> >> Discussion:
> >> https://postgr.es/m/cagtbqpa6nfgo_6g_y_7zqx8l9gchdsqkydo1tguh791z6py...@mail.gmail.com
> >
> > I don't think this warrants inclusion in the release notes for reasons
> > already discussed.  The vacuum truncation operation is a rare one and
> > an implementation detail.
> 
> \_(0_0)_/
> 
> As you wish.
> 
> Though if I wasn't already aware of it, I would like to know, because
> it's been a source of trouble in the past.

Understood, but the question is whether the release notes are the right
place to educate users of something that will no longer be a problem.  I
am happy to adjust things to whatever the community wants, but, on the
other hand I have a responsibility to be consistent what what they have
wanted in the past.  I am also open to reviewing how we filter things
compared other projects.

On a larger note, not being in the release notes doesn't mean it isn't
important, but rather, that is isn't a change users need to know about.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

-- 
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 10 release notes

On 2017-04-25 13:11:32 -0400, Bruce Momjian wrote:
> On Tue, Apr 25, 2017 at 01:37:13PM -0300, Claudio Freire wrote:
> > The truncate scan has been measured to be five times faster than without
> > this patch (that was on a slow disk, but it shouldn't hurt on fast
> > disks.)
> > 
> > Author: Álvaro Herrera, loosely based on a submission by Claudio Freire
> > Discussion:
> > https://postgr.es/m/cagtbqpa6nfgo_6g_y_7zqx8l9gchdsqkydo1tguh791z6py...@mail.gmail.com
> 
> I don't think this warrants inclusion in the release notes for reasons
> already discussed.  The vacuum truncation operation is a rare one and
> an implementation detail.

I think that's backwards. The truncate operation is quite delicate
because it happens with AccessExclusiveLock held.  This regularly does
cause issues in production.  When users look for things they possibly
should update for, something like "performance improvents in final
vacuum phase" + oneliner is going to be a lot more interesting than,
say, "Add MONEY operators for multiplication and division with INT8
values".

More and more users are going to be primarily interested in three
classes of new things in postgres: 1) performance 2) replication 3)
easier management. Arbitrarily excluding one of the major categories
from release notes isn't a useful policy, especially if we continue to
list new feature that'll effect no more than a handful of people.

- Andres

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

Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Claudio Freire

On Tue, Apr 25, 2017 at 2:11 PM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 01:37:13PM -0300, Claudio Freire wrote:
>> > I think it has been pretty common to accumulate a lot of such changes
>> > into generic entries like, say, "speedups for hash joins".  More detail
>> > than that simply isn't useful to end users; and as a rule, our release
>> > notes are too long anyway.
>>
>> In that spirit, the truncation speedups it seems are missing:
>>
>> Might be summarized simply as:
>>
>> Vacuum truncation has been sped up for rotating media, sometimes
>> considerably (up to five times in certain configurations).
>>
>> Full commit, for reference:
>>
>> commit 7e26e02eec90370dd222f35f00042f8188488ac4
>> Author: Alvaro Herrera 
>> Date:   Mon Jan 23 12:55:18 2017 -0300
>>
>> Prefetch blocks during lazy vacuum's truncation scan
>>
>> Vacuum truncation scan can be sped up on rotating media by prefetching
>> blocks in forward direction.  That makes the blocks already present in
>> memory by the time they are needed, while also letting OS read-ahead
>> kick in.
>>
>> The truncate scan has been measured to be five times faster than without
>> this patch (that was on a slow disk, but it shouldn't hurt on fast
>> disks.)
>>
>> Author: Álvaro Herrera, loosely based on a submission by Claudio Freire
>> Discussion:
>> https://postgr.es/m/cagtbqpa6nfgo_6g_y_7zqx8l9gchdsqkydo1tguh791z6py...@mail.gmail.com
>
> I don't think this warrants inclusion in the release notes for reasons
> already discussed.  The vacuum truncation operation is a rare one and
> an implementation detail.

\_(0_0)_/

As you wish.

Though if I wasn't already aware of it, I would like to know, because
it's been a source of trouble in the past.


-- 
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 10 release notes

On Tue, Apr 25, 2017 at 10:00:52AM -0700, Andres Freund wrote:
> at this point, you can see, we've squarely left O(N) country, and
> entered the vast O(N^2) waste.
> 
> 10  109ms
> 10  100  22ms
> 10  1000162ms
> 10  1  1497ms
> 10  1017260ms
> 10  2039275ms
> 
> Here we roughly stay in O(N).
> 
> (there's some other suboptimal behaviour in smgrclose(); but that's for
> another day)

OK, I got it now. :-)  Here is the new item:

Improve table creation speed in sessions that reference many
relations (Aleksander Alekseev)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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 10 release notes

On Tue, Apr 25, 2017 at 01:06:05PM -0400, Tels wrote:
> Moin,
> 
> On Mon, April 24, 2017 9:31 pm, Bruce Momjian wrote:
> > I have committed the first draft of the Postgres 10 release notes.  They
> > are current as of two days ago, and I will keep them current.  Please
> > give me any feedback you have.
> 
> Thank you! Here is one thing I noticed:
> 
> "By default planning and execution is display by EXPLAIN ANALYZE and not
> display in other cases. The new EXPLAIN option SUMMARY allows explicit
> control of this."
> 
> The grammar sounds a bit off, I'd suggest:
> 
> "By default planning and execution time are both displayed by EXPLAIN
> ANALYZE and not displayed in other cases. The new EXPLAIN option SUMMARY
> allows explicit control of this feature."

I think all that was missing was "time":

By default planning and execution time is display by
EXPLAIN ANALYZE and not display in other cases.
The new EXPLAIN option SUMMARY allows
explicit control of this.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] pgbench tap tests & minor fixes

2017-04-25 Thread Fabien COELHO



Hello Robert,

1. This patch makes assorted cosmetic and non-cosmetic changes to 
pgbench.c. That is not expected for a testing patch.


Indeed, cosmetic changes should be avoided.


If those changes need to be made because they are bug fixes or whatever,


Yep, this is the case, minor bugs, plus comment I added to make clear 
things I had to guess when doing the debug. There are thread & clients 
stats, and they were not well managed.



then they should be committed separately.


Fine with me. Note that the added tests check that the bugs/issues are 
fixed. That would suggest (1) apply the pretty small bug fixes to pgbench 
and (2) add the test, in order to avoid adding non working tests, or 
having to change test afterwards.


A bunch of them look cosmetic and could be left out or all committed 
together, according to the committer's discretion, but the functional 
ones shouldn't just be randomly included into a commit that says "add 
TAP tests for pgbench".


The only real cosmectic one is the "." added to the comment. Others are 
useful comments which helped debug.


2. I agree that the way the expression evaluation tests are structured, 
with lots of small files, is not great. The problem with it in my view 
is not so much that it creates a lot of small files, but that you end up 
with half of the test definition in 001_pgbench.pl and the other half 
spread across all of those small files.


Yep.

It'd be easy to end up with those things getting out of sync (small 
files that aren't in @errors, for example)


Hmmm. They are not expected to change, mostly new tests and files should 
be added when new features are added.


and isn't real evident on a quick glance how those files actually get 
used.


Sure. This is also more or less true of existing tests and has not been a 
problem before.


I think it would be better to move the expected output into @errors 
instead of having a separate file for it in each case.


The files do not contain the "expected output", they are the pgbench 
scripts to run through pgbench with the -f option.


The problem I see with the alternative is to have the contents of plenty 
pgbench scripts (sql comments + sql commands + backslash commands) stored 
in the middle of a perl script making it pretty unreadable, having to 
write and remove files on each test, having problems with running a given 
test again if it fails because the needed files are not there and have to 
be thought for in the middle of the perl script or not have been cleaned 
up by the test script which is not very clean...


So between "not great" and "quite bad", I chose "not great". I can do 
"quite bad", but I would prefer to avoid it:-)



3. The comment for check_pgbench_logs() is just "... with logs",
which, at least to me, is not at all clear.


Indeed.

4. With this patch, we end up with 001_pgbench.pl and 002_basic.pl. Now, 
those file names seem completely uninformative to me.


I can rename them. "001_pgbench.pl" is just the pre-existing one that I 
just edited. The "basic" is a new one with basic option error tests, 
replicating what is done in other TAP tests.



would provide about the same amount of information; I think we can do
better.  Maybe call it 002_without_server or something; that actually
explains the distinction.


Ok. Why not.


5. I don't think adding something like command_likes() is a problem
particularly; it's similar to command_like() which we have already
got, and seems like a reasonable extension of the same idea.  But I
don't like the fact that the code looks quite different,


Indeed, I put comments:-) Otherwise it does not do the same thing.

and it seems like it might be better to just extend command_like() and 
command_fails_like to each allow $expected_stdout to optionally be an 
array of patterns that are tested in turn rather than just a single 
pattern.  That seems better than adding another very similar function.


It is not so similar: I want to test (1) precise exit status, (2) one or 
range of re on stdout, and (3) one or range of re on stderr.


The available commands just allow to check stdout once if status is ok, OR 
stderr if status is not ok. No functions check for the precise status, no 
functions check for a range of re, no functions checks for both stdout & 
stderr, or only in special cases (help, version, invalid option...).


Basically what I do does not fit any function prototype cleanly, so adding 
a new functions looked like the right approach. Probably a better name 
could be thought of.


--
Fabien.


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

On Tue, Apr 25, 2017 at 12:26 PM, Stephen Frost  wrote:
> Interesting.  Seems like the question is really what we mean by "ONLY"
> here.  For my 2c, at least, if we can check that all of the partitions
> already have the constraint enforced, such that the only thing we're
> changing is the partitioned table, then that's exactly what "ONLY"
> should do.  That would require a bit of rework, presumably, of how that
> keyword is handled but the basics are all there.

Well, I'm not saying that couldn't be done, but it doesn't add any
capability that we don't have today, so from my point of view it seems
fairly pointless.  However, we can leave that for another day; for
now, I think we should focus on fixing the pg_dump bugs that this
thread is about.

> I'm planning to push just this patch to make the backend accept the
> ALTER TABLE ONLY when no partitions exist later this afternoon, ...

Cool.

> but the
> work here isn't done as noted in my other email.

Right.  I'd just like to get the work done as soon as we reasonably
can.  Tempus fugit, and all that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] PG 10 release notes

On Tue, Apr 25, 2017 at 01:37:13PM -0300, Claudio Freire wrote:
> > I think it has been pretty common to accumulate a lot of such changes
> > into generic entries like, say, "speedups for hash joins".  More detail
> > than that simply isn't useful to end users; and as a rule, our release
> > notes are too long anyway.
> 
> In that spirit, the truncation speedups it seems are missing:
> 
> Might be summarized simply as:
> 
> Vacuum truncation has been sped up for rotating media, sometimes
> considerably (up to five times in certain configurations).
> 
> Full commit, for reference:
> 
> commit 7e26e02eec90370dd222f35f00042f8188488ac4
> Author: Alvaro Herrera 
> Date:   Mon Jan 23 12:55:18 2017 -0300
> 
> Prefetch blocks during lazy vacuum's truncation scan
> 
> Vacuum truncation scan can be sped up on rotating media by prefetching
> blocks in forward direction.  That makes the blocks already present in
> memory by the time they are needed, while also letting OS read-ahead
> kick in.
> 
> The truncate scan has been measured to be five times faster than without
> this patch (that was on a slow disk, but it shouldn't hurt on fast
> disks.)
> 
> Author: Álvaro Herrera, loosely based on a submission by Claudio Freire
> Discussion:
> https://postgr.es/m/cagtbqpa6nfgo_6g_y_7zqx8l9gchdsqkydo1tguh791z6py...@mail.gmail.com

I don't think this warrants inclusion in the release notes for reasons
already discussed.  The vacuum truncation operation is a rare one and
an implementation detail.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Cached plans and statement generalization

On Tue, Apr 25, 2017 at 9:45 AM, Konstantin Knizhnik 
 wrote: On 25.04.2017 19:12, Serge Rielau wrote:

On Apr 25, 2017, at 8:11 AM, Konstantin Knizhnik < k.knizh...@postgrespro.ru 
[k.knizh...@postgrespro.ru] > wrote: Another problem is caused by using integer 
literals in context where parameters can not be used, for example "order by 1”.
You will also need to deal with modifiers in types such as VARCHAR(10). Not 
sure if there are specific functions which can only deal with literals (?) as 
well. Sorry, I do not completely understand how presence of type modifiers can 
affect string literals used in query.
Can you provide me some example? SELECT ‘hello’::CHAR(10) || ‘World’, 5 + 6;
You can substitute ‘hello’, ‘World’, 5, and 6. But not 10.
Also some OLAP syntax like “rows preceding”
It pretty much boils down to whether you can do some shallow parsing rather 
than expending the effort to build the parse tree.
Cheers Serge

Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Tels

Moin,

On Mon, April 24, 2017 9:31 pm, Bruce Momjian wrote:
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.

Thank you! Here is one thing I noticed:

"By default planning and execution is display by EXPLAIN ANALYZE and not
display in other cases. The new EXPLAIN option SUMMARY allows explicit
control of this."

The grammar sounds a bit off, I'd suggest:

"By default planning and execution time are both displayed by EXPLAIN
ANALYZE and not displayed in other cases. The new EXPLAIN option SUMMARY
allows explicit control of this feature."

Regards,

Tels

-- 
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 10 release notes

On 2017-04-25 10:10:07 -0400, Bruce Momjian wrote:
> On Mon, Apr 24, 2017 at 08:52:05PM -0700, Andres Freund wrote:
> > On 2017-04-24 23:45:06 -0400, Tom Lane wrote:
> > Oh, I completely agree with accumulating related changes, and that
> > code-level details aren't useful.  I think we skipped them entirely
> > here.  And I just listed my own changes because I could find them
> > quickly, but they're not alone, e.g:
> > 090010f2ec9b1f9ac1124dc628b89586f911b641 - Improve performance of 
> > find_tabstat_entry()/get_tabstat_entry()
> > which makes it realistic to have sessions touching many relations, which
> > previously was O(#relations^2), and which caused repeated complaints
> > over the years, and allows for different usecases.
> 
> Looking at this commit it appears to improve pg_stat statistics handling.
> I don't see how that improves performance except to improve statistics
> aggregation, which happens in the statistics process.

Previously when creating a new relation, we had to walk a list of all
open relations (i.e. O(N) work for each relation), now it's O(1) for
each relation.  And that happens in the backends, not in the statistics
collector.  It's pretty easy to see the effect.  Write a plpgsql
function that creates, say, 100k tables. Run it in 9.6 and v10.

CREATE OR REPLACE FUNCTION create_tables(p_ntables int) RETURNS void LANGUAGE 
plpgsql AS $$DECLARE i int;BEGIN FOR i IN 1 .. p_ntables LOOP EXECUTE 'CREATE 
TABLE tbl_'||i::text||'();';END LOOP;END;$$

Measuring the time for the SELECT in
BEGIN;SELECT create_tables(1);ROLLBACK;

Recreating the server inbetween each run I get:

version  #nrelstime
9.6  107ms
9.6  100  23ms
9.6  1000159ms
9.6  1  1465ms
9.6  1032367ms
9.6  20   144026ms

at this point, you can see, we've squarely left O(N) country, and
entered the vast O(N^2) waste.

10  109ms
10  100  22ms
10  1000162ms
10  1  1497ms
10  1017260ms
10  2039275ms

Here we roughly stay in O(N).

(there's some other suboptimal behaviour in smgrclose(); but that's for
another day)

- Andres

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

Re: [HACKERS] Cached plans and statement generalization

2017-04-25 Thread David Fetter

On Tue, Apr 25, 2017 at 06:11:09PM +0300, Konstantin Knizhnik wrote:
> On 24.04.2017 21:43, Andres Freund wrote:
> > Hi,
> > 
> > On 2017-04-24 11:46:02 +0300, Konstantin Knizhnik wrote:
> > > So what I am thinking now is implicit query caching. If the same query 
> > > with
> > > different literal values is repeated many times, then we can try to
> > > generalize this query and replace it with prepared query with
> > > parameters.
> > That's not actuall all that easy:
> > - You pretty much do parse analysis to be able to do an accurate match.
> >How much overhead is parse analysis vs. planning in your cases?
> > - The invalidation infrastructure for this, if not tied to to fully
> >parse-analyzed statements, is going to be hell.
> > - Migrating to parameters can actually cause significant slowdowns, not
> >nice if that happens implicitly.
> 
> Well, first of all I want to share results I already get: pgbench with
> default parameters,  scale 10 and one connection:
> 
> protocol
>   TPS
> simple
>   3492
> extended
>   2927
> prepared
>   6865
> simple + autoprepare
>   6844

If this is string mashing on the unparsed query, as it appears to be,
it's going to be a perennial source of security issues.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Cached plans and statement generalization




On 25.04.2017 19:12, Serge Rielau wrote:


On Apr 25, 2017, at 8:11 AM, Konstantin Knizhnik 
> wrote:
Another problem is caused by using integer literals in context where 
parameters can not be used, for example "order by 1”.
You will also need to deal with modifiers in types such as 
VARCHAR(10). Not sure if there are specific functions which can only 
deal with literals (?) as well.


Sorry, I do not completely understand how presence of type modifiers can 
affect string literals used in query.

Can you provide me some example?



Doug Doole did this work in DB2 LUW and he may be able to point to 
more places to watch out for semantically.


Generally, in my experience, this feature is very valuable when 
dealing with (poorly designed) web apps that just glue together strings.


I do not think that this optimization will be useful only for poorly 
designed application.
I already pointed on two use cases where prepapred statements can not be 
used:

1. pgbouncer without session-level pooling.
2. partitioning

Protecting it under a GUC would allow to only do the work if it’s 
deemed likely to help.
Another rule I find useful is to abort any efforts to substitute 
literals if any bind variable is found in the query.
That can be used as a cue that the author of the SQL left the 
remaining literals in on purpose.


A follow up feature would be to formalize different flavors of peeking.
I.e. can you produce a generic plan, but still recruit the initial set 
of bind values/substituted literals to dos costing?

Here situation is the same as for explicitly prepared statements, isn't it?
Sometimes it is preferrable to use specialized plan rather than generic 
plan.

I am not sure if postgres now is able to do it.




Cheers
Serge Rielau
Salesforce.com 

PS: FWIW, I like this feature.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] question: data file update when pg_basebackup in progress

2017-04-25 Thread David G. Johnston

On Tue, Apr 25, 2017 at 9:08 AM, Rui Hai Jiang  wrote:

> When pg_basebackup is launched, a checkpoint is created first, then all
> files are transferred to the  pg_basebackup client.  Is it possible that a
> data page(say page-N) in a data file is changed after the checkpoint and
> before the pg_basebackup is finished?
>

I believe so.

> If this happens,  is it possible that only part of the changed page be
> transferred to the pg_basebackup client?  i.e.  the pg_basebackup client
> gets page-N with part of the old content and part of the new content. How
> does postgreSQL handle this kind of data page?
>

The first write to a page after a checkpoint is always recorded in the WAL
as a full page write.  Every WAL file since the checkpoint must also be
copied to the backed up system.  The replay of those WAL files is what
brings the remote and local system into sync with respect to all changes
since the backup checkpoint.

David J.

Re: [HACKERS] pgbench tap tests & minor fixes

2017-04-25 Thread Fabien COELHO



Hello Nikolay,


- I agree to add a generic command TestLib & a wrapper in PostgresNode,

  instead of having pgbench specific things in the later, then call
  them from pgbench test script.

- I still think that moving the pgbench scripts inside the test script
  is a bad idea (tm).


My sum up is the following:

I see my job as a reviewer is to tell "I've read the code, and I am sure 
it is good".


I'm ok with that. Does not mean I cannot argue if I happen to disagree on 
a point.



I can tell this about this code, if:

- There is no pgbench specific staff in src/test/perl. Or there should be
_really_big_ reason for it.


ISTM that v2 I sent does not contain any pgbench specific code. However It 
contains new generic code to check for status and list of re on stdout & 
stderr.



- All the testing is done via calls of TestLib.pm functions. May be these
functions should be improved somehow. May be there should be some warper
around them. But not direct IPC::Run::run call.


There is no call to IPC out of TestLib.pm in v2 I sent.


- All the pgbench scripts are stored in one file. 36 files are not acceptable.
I would include them in the test script itself. May be it can be done in other
ways. But not 36 less then 100 byte files in source code tree...


I will write an ugly stuff if it is require to pass this patch, but I'm 
unconvinced that this is a good idea.


What it is issue with having 36 small files in a directory?

Pg source tree currently contains about 79 under 100 bytes files related 
to the insufficient test it is running, so this did not seem to be an 
issue in the past.



May be I am wrong. I am not a guru. But then somebody else should say "I've
read the code, and I am sure it is good" instead of me. And it would be his
responsibility then. But if you ask me, issues listed above are very
important, and until they are solved I can not tell "the code is good", and I
see no way to persuade me. May be just ask somebody else...


Of all the issues you list, 2/3 are already fixed in the v2 I sent 
attached to the mail you are responding to, and I actually think that the 
last point is a bad idea, which I can implement and be sad about.


--
Fabien.


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

Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Claudio Freire

On Tue, Apr 25, 2017 at 12:45 AM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2017-04-24 23:37:42 -0400, Bruce Momjian wrote:
>>> I remember seeing those and those are normally details I do not put in
>>> the release notes as there isn't a clear user experience change except
>>> "Postgres is faster".  Yeah, a bummer, and I can change my filter, but
>>> it would require discussion.
>
>> I think "postgres is faster" is one of the bigger user demands, so I
>> don't think that policy makes much sense.  A large number of the changes
>> over the next few releases will focus solely on that.  Nor do I think
>> past release notes particularly filtered such changes out.
>
> I think it has been pretty common to accumulate a lot of such changes
> into generic entries like, say, "speedups for hash joins".  More detail
> than that simply isn't useful to end users; and as a rule, our release
> notes are too long anyway.

In that spirit, the truncation speedups it seems are missing:

Might be summarized simply as:

Vacuum truncation has been sped up for rotating media, sometimes
considerably (up to five times in certain configurations).

Full commit, for reference:

commit 7e26e02eec90370dd222f35f00042f8188488ac4
Author: Alvaro Herrera 
Date:   Mon Jan 23 12:55:18 2017 -0300

Prefetch blocks during lazy vacuum's truncation scan

Vacuum truncation scan can be sped up on rotating media by prefetching
blocks in forward direction.  That makes the blocks already present in
memory by the time they are needed, while also letting OS read-ahead
kick in.

The truncate scan has been measured to be five times faster than without
this patch (that was on a slow disk, but it shouldn't hurt on fast
disks.)

Author: Álvaro Herrera, loosely based on a submission by Claudio Freire
Discussion:
https://postgr.es/m/cagtbqpa6nfgo_6g_y_7zqx8l9gchdsqkydo1tguh791z6py...@mail.gmail.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_dump emits ALTER TABLE ONLY partitioned_table

2017-04-25 Thread Stephen Frost

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost  wrote:
> > I wonder why the restriction is there, which is probably part of the
> > reason that I'm thinking of phrasing the documentation that way.
> >
> > Beyond a matter of round to-its, is there a reason why it couldn't (or
> > shouldn't) be supported?  I'm not suggesting now, of course, but in a
> > future release.
> 
> I don't see any particular reason, but I haven't looked into the
> matter deeply.  One thing to consider is that you can already more or
> less do exactly that thing, like this:
> 
> rhaas=# create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> Time: 4.738 ms
> rhaas=# create table foo1 partition of foo for values from (0) to (100);
> CREATE TABLE
> Time: 5.162 ms
> rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,1000) g;
> INSERT 0 1000
> Time: 12835.153 ms (00:12.835)
> rhaas=# alter table foo1 add constraint xyz check (b != 'nope');
> ALTER TABLE
> Time: 1059.728 ms (00:01.060)
> rhaas=# alter table foo add constraint xyz check (b != 'nope');
> NOTICE:  merging constraint "xyz" with inherited definition
> ALTER TABLE
> Time: 1.243 ms
> 
> So we may not really need another way to do it.

Interesting.  Seems like the question is really what we mean by "ONLY"
here.  For my 2c, at least, if we can check that all of the partitions
already have the constraint enforced, such that the only thing we're
changing is the partitioned table, then that's exactly what "ONLY"
should do.  That would require a bit of rework, presumably, of how that
keyword is handled but the basics are all there.

> >> Also, I think saying that it it will result in an error is a bit more
> >> helpful than saying that it is is not supported, since the latter
> >> might lead someone to believe that it could result in undefined
> >> behavior (i.e. arbitrarily awful misbehavior) which is not the case.
> >> So I like what he wrote, for whatever that's worth.
> >
> > I don't mind stating that it'll result in an error, I just don't want to
> > imply that there's some way to get around that error if things were done
> > differently.
> >
> > How about:
> >
> > ---
> > Once partitions exist, using ONLY will always result
> > in an error as adding or dropping constraints on only the partitiioned
> > table, when partitions exist, is not supported.
> > ---
> 
> I still prefer Amit's language, but it's not worth to me to keep
> arguing about it.  If you prefer what you've written there, fine, but
> let's get something committed and move on.  I think we understand what
> needs to be fixed here now, and I'd like to do get that done.  If you
> don't want to do it or don't have time to do it, I'll pick it up
> again, but let's not let this keep dragging out.

I'm planning to push just this patch to make the backend accept the
ALTER TABLE ONLY when no partitions exist later this afternoon, but the
work here isn't done as noted in my other email.

Thanks!

Stephen
From c51fbaabf4da121c8bfd68b7bf74db1fbea4e581 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Mon, 17 Apr 2017 12:06:12 -0400
Subject: [PATCH] Allow ALTER TABLE ONLY on partitioned tables

There is no need to forbid ALTER TABLE ONLY on partitioned tables,
when no partitions exist yet.  This can be handy for users who are
building up their partitioned table independently and will create actual
partitions later.

In addition, this is how pg_dump likes to operate in certain instances.

Author: Amit Langote, with some error message word-smithing by me
---
 doc/src/sgml/ddl.sgml | 18 ++---
 src/backend/commands/tablecmds.c  | 66 +++
 src/test/regress/expected/alter_table.out | 36 +++--
 src/test/regress/expected/truncate.out|  8 
 src/test/regress/sql/alter_table.sql  | 24 ---
 src/test/regress/sql/truncate.sql |  4 ++
 6 files changed, 108 insertions(+), 48 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961..84c4f20 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,23 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions.  CHECK constraints that are marked
-   NO INHERIT are not allowed.
+   NO INHERIT are not allowed to be created on
+   partitioned tables.
   
  
 
  
   
-   The ONLY notation used to exclude child tables
-   will cause an error for partitioned tables in the case of
-   schema-modifying commands such as most ALTER TABLE
-   commands.  For example, dropping a column from only the parent does
-   not make sense for partitioned tables.
+   Using ONLY to add or drop a constraint on only the
+   partitioned table is supported when there are no

Re: [HACKERS] Cached plans and statement generalization