[HACKERS] Commitfest 201709 is now closed

[Daniel Gustafsson]

We have now entered October, which marks the end of Commitfest 201709.  All
patches have now been dealt with and the commitfest is closed.  Before starting
on moving, and closing, patches the stat for this commitfest were:

Needs review: 69.
Waiting on Author: 22.
Ready for Committer: 40.
Committed: 88.
Moved to next CF: 11.
Rejected: 8.
Returned with Feedback: 18.
===
Total: 256.

29 patches were in need of review but didn’t have a reviewer assigned (and no
review posted och -hackers), these have all been moved to the next CF.  On top
of that, 40 patches were Ready for committer, and have been moved to the next
commitfest.  The next commitfest is thus quite the smorgasbord for committers
already.

A few patches had been "Waiting for author" during the entire commitfest
without seeing any updates, and were thus closed as “Returned with feddback”.
A few others patches were Returned with feedback too, due to either not moving
from Waiting for author, or due to review comments being made.

And last, but not least, a lot of patches were pushed to the next commitfest.
What I’ve tried to do is move patches such that every patch submitted has a
fair chance at a good review.  Also, patches being actively discussed and
worked on were moved of course.  With the volumes in mind, I’m absolutely
certain I’ve botched one or two up though.  If you have a patch that was moved,
and you intend to rewrite enough of it to warrant a resubmission then please go
in and close your entry.

In summary, this commitfest saw a large number of patches, and while lots of
patches got reviewed and committed, there is a lot of improvement to be had
when it comes to closing them.  We clearly need more bandwidth among reviewers
and committers to cope with the increasing size of the commitfests.  There is a
clear risk that too much gets moved to the next commitfest making each new
commitfest progressively larger and harder to handle.  I don’t have any good
answers, but we probably need to think of something going forward.

Thanks to everyone who participated, and to everyone who have responded to my
nagging via the CF app email function. This is clearly an awesome community.

cheers ./daniel

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

Re: [HACKERS] Fix number skipping in to_number

[Daniel Gustafsson]

> On 25 Sep 2017, at 02:52, Nathan Wagner  wrote:
> 
> On Thu, Aug 17, 2017 at 12:33:02PM +0100, Oliver Ford wrote:
> 
>> Ok I've made that change in the attached v3. I'm not sure as I'm on
>> en_US.UTF-8 locale too. Maybe something Windows specific?
> 
> This patch applies against master (8485a25a), compiles, and
> passes a make check.
> 
> I tested both on my mac laptop, and my linux server.
> 
> If we want this patch, I'd say it's ready for committer.  We may want
> (and I can't believe I'm saying this) more discussion as to exactly what
> the strategy for to_number() (and friends) is.

Based on this review, I am moving this patch to the next commitfest and will
update it to Ready for committer.  There might, as you say, be more discussion
needed, but due to the lack of extensive such so far I’ll move it for now.

cheers ./daniel

-- 
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] generated columns

[Daniel Gustafsson]

> On 12 Sep 2017, at 21:35, Jaime Casanova  
> wrote:
> 
> On 10 September 2017 at 00:08, Jaime Casanova
>  wrote:
>> 
>> During my own tests, though, i found some problems:
> 
> a few more tests:
> 
> create table t1 (
> id serial,
> height_cm int,
> height_in int generated always as (height_cm * 10)
> ) ;
> 
> 
> """
> postgres=# alter table t1 alter height_cm type numeric;
> ERROR:  unexpected object depending on column: table t1 column height_in
> """
> should i drop the column and recreate it after the fact? this seems
> more annoying than the same problem with views (drop view & recreate),
> specially after you implement STORED
> 
> 
> """
> postgres=# alter table t1 alter height_in type numeric;
> ERROR:  found unexpected dependency type 'a'
> """
> uh!?
> 
> 
> also is interesting that in triggers, both before and after, the
> column has a null. that seems reasonable in a before trigger but not
> in an after trigger
> """
> create function f_trg1() returns trigger as $$
>  begin
> raise notice '%', new.height_in;
> return new;
>  end
> $$ language plpgsql;
> 
> create trigger trg1 before insert on t1
> for each row execute procedure f_trg1();
> 
> postgres=# insert into t1 values(default, 100);
> NOTICE:  
> INSERT 0 1
> 
> create trigger trg2 after insert on t1
> for each row execute procedure f_trg1();
> 
> postgres=# insert into t1 values(default, 100);
> NOTICE:  
> NOTICE:  
> INSERT 0 1
> """
> 
> the default value shouldn't be dropped.
> """
> postgres=# alter table t1 alter height_in drop default;
> ALTER TABLE
> postgres=# \d t1
>  Table "public.t1"
>  Column   |  Type   | Collation | Nullable |Default
> +-+---+--+
> id | integer |   | not null |
> nextval('t1_id_seq'::regclass)
> height_cm | integer |   |  |
> height_in   | integer |   |  | generated always as ()
> “""

Based on this review, and the errors noted in upthread in the previous review,
I’m marking this Returned with feedback.  When an updated version of the patch
is ready, please re-submit it to an upcoming commitfest.

cheers ./daniel

-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Daniel Gustafsson]

> On 15 Sep 2017, at 16:36, Bossart, Nathan  wrote:
> 
> A few general comments.
> 
> While this patch applies, I am still seeing some whitespace errors:
> 
>  comment_on_current_database_no_pgdump_v4.1.patch:488: trailing whitespace.
>   ColId
>  comment_on_current_database_no_pgdump_v4.1.patch:490: trailing whitespace.
>   | CURRENT_DATABASE
>  comment_on_current_database_no_pgdump_v4.1.patch:491: trailing whitespace.
>   {
>  comment_on_current_database_no_pgdump_v4.1.patch:501: trailing whitespace.
>   ColId
>  comment_on_current_database_no_pgdump_v4.1.patch:502: trailing whitespace.
>   {
>  warning: squelched 9 whitespace errors
>  warning: 14 lines add whitespace errors.
> 
> It looks like the 'dbname' test is currently failing when you run
> 'make check-world'.  The Travis CI build log [1] shows the details
> of the failure.  From a brief glance, I would guess that some of
> the queries are returning unexpected databases that are created in
> other tests.
> 
> Also, I think this change will require updates to the
> documentation.
> 
> + if (dbspecname->dbnametype == DBSPEC_CURRENT_DATABASE )
> + dbname = get_database_name(MyDatabaseId);
> + else
> + dbname = dbspecname->dbname;
> 
> This pattern is repeated in the patch several times.  It looks like
> the end goal of these code blocks is either to get the database
> name or the database OID, so perhaps we should have
> get_dbspec_name() and get_dbspec_oid() helper functions (like
> get_rolespec_oid() for RoleSpec nodes).
> 
> +static bool
> +_equalDatabaseSpec(const DBSpecName *a, const DBSpecName *b)
> +{
> + COMPARE_SCALAR_FIELD(dbnametype);
> + COMPARE_STRING_FIELD(dbname);
> + COMPARE_LOCATION_FIELD(location);
> +
> + return true;
> +}
> 
> There are some inconsistencies in the naming strategy.  For
> example, this function is called _equalDatabaseSpec(), but the
> struct is DBSpecName.  I would suggest calling it DatabaseSpec or
> DbSpec throughout the patch.

Based on this review, and that there hasn’t been a new version submitted, I’m
marking this patch Returned with Feedback.  Please re-submit a new version of
the patch to an upcoming commitfest when ready.

cheers ./daniel

-- 
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] GSoC 2017: weekly progress reports (week 8)

[Daniel Gustafsson]

> On 29 Sep 2017, at 00:59, Alexander Korotkov  
> wrote:
> 
> On Thu, Sep 28, 2017 at 2:22 PM, Alexander Korotkov 
> > wrote:
> On Fri, Jul 28, 2017 at 7:58 AM, Shubham Barai  > wrote:
> I am attaching a patch for predicate locking in SP-Gist index
> 
> I took a look over this patch.
> 
>   newLeafBuffer = SpGistGetBuffer(index,
>   
> GBUF_LEAF | (isNulls ? GBUF_NULLS : 0),
>   
> Min(totalLeafSizes,
>   
> SPGIST_PAGE_CAPACITY),
>   
> );
>   PredicateLockPageSplit(index,
>   BufferGetBlockNumber(current->buffer),
>   BufferGetBlockNumber(newLeafBuffer));
> 
> You move predicate lock during split only when new leaf page is allocated.  
> However SP-GiST may move items to the free space of another busy page during 
> split (see other branches in doPickSplit()).  Your patch doesn't seem to 
> handle this case correctly.
> 
> I've more thoughts about this patch.
> 
> + * SPGist searches acquire predicate lock only on the leaf pages.
> + During a page split, a predicate lock is copied from the original
> + page to the new page.
> 
> This approach isn't going to work.  When new tuple is inserted into SP-GiST, 
> choose method can return spgAddNode.  In this case new branch of the tree is 
> added.  And this new branch could probably be used by scans we made in the 
> past.  Therefore, you need to do predicate locking for internal pages too in 
> order to detect all the possible conflicts.

Based on this review, I’ve marked this patch Returned with feedback.  Please
re-submit a new version to an upcoming commitfest when ready.

cheers ./daniel

-- 
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] Transactions involving multiple postgres foreign servers

[Daniel Gustafsson]

> On 02 Oct 2017, at 08:31, Masahiko Sawada  wrote:
> 
> On Sat, Sep 30, 2017 at 12:42 AM, Robert Haas  wrote:
>> On Wed, Sep 27, 2017 at 11:15 PM, Masahiko Sawada  
>> wrote:
>>> I think that making a resolver process have connection caches to each
>>> foreign server for a while can reduce the overhead of connection to
>>> foreign servers. These connections will be invalidated by DDLs. Also,
>>> most of the time we spend to commit a distributed transaction is the
>>> interaction between the coordinator and foreign servers using
>>> two-phase commit protocal. So I guess the time in signalling to a
>>> resolver process would not be a big overhead.
>> 
>> I agree.  Also, in the future, we might try to allow connections to be
>> shared across backends.  I did some research on this a number of years
>> ago and found that every operating system I investigated had some way
>> of passing a file descriptor from one process to another -- so a
>> shared connection cache might be possible.
> 
> It sounds good idea.
> 
>> Also, we might port the whole backend to use threads, and then this
>> problem goes way.  But I don't have time to write that patch this
>> week.  :-)
>> 
>> It's possible that we might find that neither of the above approaches
>> are practical and that the performance benefits of resolving the
>> transaction from the original connection are large enough that we want
>> to try to make it work anyhow.  However, I think we can postpone that
>> work to a future time.  Any general solution to this problem at least
>> needs to be ABLE to resolve transactions at a later time from a
>> different session, so let's get that working first, and then see what
>> else we want to do.
> 
> I understood and agreed. I'll post the first version patch of new
> design to next CF.

Closing this patch with Returned with feedback in this commitfest, looking
forward to a new version in an upcoming commitfest.

cheers ./daniel

-- 
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] Hooks to track changed pages for backup purposes

[Daniel Gustafsson]

> On 13 Sep 2017, at 15:01, Tomas Vondra  wrote:
> 
> On 09/13/2017 07:53 AM, Andrey Borodin wrote:
>>> * I see there are conditions like this:
>>> 
>>>if(xlogreader->blocks[nblock].forknum == MAIN_FORKNUM)
>>> 
>>> Why is it enough to restrict the block-tracking code to main fork?
>>> Aren't we interested in all relation forks?
>> fsm, vm and others are small enough to take them 
> 
> That seems like an optimization specific to your backup solution, not
> necessarily to others and/or to other possible use cases.
> 
>>> I guess you'll have to explain
>>> what the implementation of the hooks is supposed to do, and why these
>>> locations for hook calls are the right ones. It's damn impossible to
>>> validate the patch without that information.
>>> 
>>> Assuming you still plan to use the hook approach ...
>> Yes, I still think hooking is good idea, but you are right - I need
>> prototype first. I'll mark patch as Returned with feedback before
>> prototype implementation.
> 
> OK
> 
> There
> are no arguments fed to this hook, so modules would not be able to
> analyze things in this context, except shared memory and process
> state?
 
> 
> Those hooks are put in hot code paths, and could impact performance of
> WAL insertion itself.
 I do not think sending few bytes to cached array is comparable to disk
>>> write of XLog record. Checking the func ptr is even cheaper with correct
>>> branch prediction.
>>> 
>>> That seems somewhat suspicious, for two reasons. Firstly, I believe we
>>> only insert the XLOG records into WAL buffer here, so why should there
>>> be any disk write related? Or do you mean the final commit?
>> Yes, I mean finally we will be waiting for disk. Hundred empty ptr
>> checks are neglectable in comparision with disk.
> 
> Aren't we doing these calls while holding XLog locks? IIRC there was
> quite a significant performance improvement after Heikki reduced the
> amount of code executed while holding the locks.
> 
>>> But more importantly, doesn't this kind of information require some
>>> durability guarantees? I mean, if it gets lost during server crashes or
>>> restarts, doesn't that mean the incremental backups might miss some
>>> buffers? I'd guess the hooks will have to do some sort of I/O, to
>>> achieve that, no?
>> We need durability only on the level of one segment. If we do not have
>> info from segment we can just rescan it.
>> If we send segment to S3 as one file, we are sure in it's integrity. But
>> this IO can by async.
>> 
>> PTRACK in it's turn switch bits in fork's buffers which are written in
>> checkpointer and..well... recovered during recovery. By usual WAL replay
>> of recovery.
> 
> But how do you do that from the hooks, if they only store the data into
> a buffer in memory? Let's say you insert ~8MB of WAL into a segment, and
> then the system crashes and reboots. How do you know you have incomplete
> information from the WAL segment?
> 
> Although, that's probably what wal_switch_hook() might do - sync the
> data whenever the WAL segment is switched. Right?
> 
>>> From this POV, the idea to collect this information on the backup system
>>> (WAL archive) by pre-processing the arriving WAL segments seems like the
>>> most promising. It moves the work to another system, the backup system
>>> can make it as durable as the WAL segments, etc.
>> 
>> Well, in some not so rare cases users encrypt backups and send to S3.
>> And there is no system with CPUs that can handle that WAL parsing.
>> Currently, I'm considering mocking prototype for wal-g, which works
>> exactly this.
> 
> Why couldn't there be a system with enough CPU power? Sure, if you want
> to do this, you'll need a more powerful system, but regular CPUs can do
>> 1GB/s in AES-256-GCM thanks to AES-NI. Or you could do it on the
> database as part of archive_command, before the encryption, of course.

Based on unanswered questions in the discussion in this thread, and that no new
version of the patch has been submitted, I’m marking this returned with
feedback.  Please re-submit the patch in a future commitfest when ready for new
review.

cheers ./daniel

-- 
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] Making clausesel.c Smarter

[Daniel Gustafsson]

> On 07 Sep 2017, at 09:30, Daniel Gustafsson <dan...@yesql.se> wrote:
> 
>> On 06 Sep 2017, at 07:13, David Rowley <david.row...@2ndquadrant.com> wrote:
>> 
>> On 6 September 2017 at 00:43, Daniel Gustafsson <dan...@yesql.se> wrote:
>>> This patch was moved to the currently open Commitfest.  Given the above
>>> comment, is the last patch in this thread still up for review, or are you
>>> working on a new version?  Just to avoid anyone reviewing an outdated 
>>> version.
>> 
>> Hi Daniel,
>> 
>> I've not had time to work on a new version yet and I can't imagine
>> that I will for quite some time.
>> 
>> The idea I had to remove the quadratic search on the list was to add
>> to or modify equal() in equalfuncs.c to have it return -1, 0, +1 and
>> use that as a comparison function in a binary search tree. The Btree
>> would complement List as a way of storing Nodes in no particular
>> order, but in a way that a Node can be found quickly. There's likely
>> more than a handful of places in the planner that would benefit from
>> this already.
>> 
>> It's not a 5-minute patch and it probably would see some resistance,
>> so won't have time to look at this soon.
>> 
>> If the possibility of this increasing planning time in corner cases is
>> going to be a problem, then it might be best to return this with
>> feedback for now and I'll resubmit if I get time later in the cycle.
> 
> For now I’ve marked this “Waiting on Author” awaiting a new patch for the
> community to consider.  If there is no time to hack on this in the current CF
> (which seems likely given your mail above) I’ll move it to the next CF as this
> one draws to a close.

Going back on my previous thinking of moving to the next CF, I am instead
marking this as returned with feedback.  When a new version of the patch is
ready, please re-submit to a future commitfest.

cheers ./daniel

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

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

[Daniel Gustafsson]

> On 20 Sep 2017, at 00:29, Jacob Champion  wrote:
> 
> On Wed, Sep 6, 2017 at 8:37 AM, Jacob Champion  wrote:
>> On Tue, Sep 5, 2017 at 10:49 PM, Michael Paquier
>>  wrote:
>>> In short, it seems to me that this patch should be rejected in its
>>> current shape.
>> 
>> Is the half of the patch that switches PageGetLSN to
>> BufferGetLSNAtomic correct, at least?
> 
> Any further thoughts on this? If the BufferGetLSNAtomic fixes made
> here are not correct to begin with, then the rest of the patch is
> probably moot; I just want to double-check that that is the case.

Based on the discussions in this thread, I’m marking this patch Returned with
feedback. Please re-submit a new version in an upcoming commitfest when ready.

cheers ./daniel

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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Daniel Gustafsson]

> On 28 Jul 2017, at 16:46, Robert Haas  wrote:
> 
> On Fri, Jul 28, 2017 at 12:39 AM, Pavan Deolasee
>  wrote:
>> I see your point. But I would like to think this way: does the technology
>> significantly help many common use cases, that are currently not addressed
>> by HOT? It probably won't help all workloads, that's given. Also, we don't
>> have any credible alternative while this patch has progressed quite a lot.
>> May be Robert will soon present the pluggable storage/UNDO patch and that
>> will cover everything and more that is currently covered by HOT/WARM. That
>> will probably make many other things redundant.
> 
> A lot of work is currently being done on this, by multiple people,
> mostly not including me, and a lot of good progress is being made.
> But it's not exactly ready to ship, nor will it be any time soon.  I
> think we can run a 1-client pgbench without crashing the server at
> this point, if you tweak the configuration a little bit and don't do
> anything fancy like, say, try to roll back a transaction.  :-)

The discussions in this implies that there is a bit more work on this patch,
which also hasn’t moved in the current commitfest, so marking it Returned with
Feedback.  Please re-submit this work in a future commitfest when ready for a
new round of reviews.

cheers ./daniel

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

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Daniel Gustafsson]

> On 13 Sep 2017, at 07:44, Vaishnavi Prabakaran 
>  wrote:
> 
> On Wed, Sep 13, 2017 at 3:33 PM, Craig Ringer  > wrote:
> 
> I really do not like calling it "commit" as that conflates with a database 
> commit.
> 
> A batch can embed multiple BEGINs and COMMITs. It's entirely possible for an 
> earlier part of the batch to succeed and commit, then a later part to fail, 
> if that's the case. So that name is IMO wrong.
> 
> Ok, SendQueue seems ok to me as well. Will change it in next version. 
>  
> +"a"?
> 
> Hmm, Can you explain the question please. I don't understand.
> 
> s/of new query/of a new query/
> 
> Thanks for explaining. Will change this too in next version. 

Based on the discussions in this thread, and that a new version hasn’t been
submitted, I’m marking this Returned with Feedback.  Please re-submit the new
version in an upcoming commitfest when ready.

cheers ./daniel

-- 
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] Parallel COPY FROM execution

[Daniel Gustafsson]

> On 11 Aug 2017, at 20:07, Robert Haas  wrote:
> 
> On Fri, Aug 11, 2017 at 9:55 AM, Alex K  wrote:
>> - I have used both Latch and ConditionalVariable for the same
>> purpose–wait until some signal
>>  occurs–and for me as an end user they perform quite similar. I
>> looked into the condition_variable.c
>>  code and it uses Latch and SpinLock under the hood. So what are
>> differences and dis-/advantages
>>  between Latch and ConditionalVariable?
> 
> A ConditionVariable lets you signal the processes that are waiting
> without needing to know in advance exactly which processes those are.
> If you use latches directly, you'll have to somehow keep track of
> which processes need to be signaled.

Based on the discussion in this thread, and that a new version of the patch
hasn’t been submitted during the commitfest, I’m marking this Returned with
Feedback.  Please re-submit a new version in an upcoming commitfest.

cheers ./daniel

-- 
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] Explicit relation name in VACUUM VERBOSE log

[Daniel Gustafsson]

> On 29 Aug 2017, at 17:21, Robert Haas  wrote:
> 
> On Tue, Aug 22, 2017 at 2:23 AM, Simon Riggs  wrote:
>> Yes, we can. I'm not sure why you would do this only for VACUUM
>> though? I see many messages in various places that need same treatment
> 
> I'm skeptical about the idea of doing this too generally.
> 
> rhaas=> select * from foo;
> ERROR:  permission denied for relation foo
> 
> Do we really want to start saying public.foo in all such error
> messages?  To me, that's occasionally helpful but more often just
> useless chatter.
> 
> One problem with making this behavior optional is that we'd then need
> two separate translatable strings in every case, one saying "table %s
> has problems" and the other saying "table %s.%s has problems".  Maybe
> we could avoid that via some clever trick, but that's how we're doing
> it in some existing cases.
> 
> I have a feeling we're making a small patch with a narrow goal into a
> giant patch for which everyone has a different goal.

Based on the concerns raised in this thread which are left to address or
resolve, and the fact that the patch has been without update during the CF, I’m
marking this Returned with Feedback.  Please re-submit a new version of the
patch to a future commitfest.

cheers ./daniel

-- 
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] document and use SPI_result_code_string()

[Daniel Gustafsson]

> On 06 Sep 2017, at 14:25, Tom Lane  wrote:
> 
> Michael Paquier  writes:
>> Fine for 0002. This reminds me of LockGXact and RemoveGXact in
>> twophase.c, as well as _hash_squeezebucket that have some code paths
>> that cannot return... Any thoughts about having some kind of
>> PG_NOTREACHED defined to 0 which could be put in an assertion?
> 
> Generally we just do "Assert(false)", maybe with "not reached" in a
> comment.  I don't feel a strong need to invent a new way to do that.

Moving this to the next commitfest and bumping status to Ready for committer
based on the discussion in this thread.

cheers ./daniel


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

Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Daniel Gustafsson]

> On 22 Sep 2017, at 18:57, Melanie Plageman  wrote:
> 
> On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman  > wrote:
> The latest patch applies cleanly and builds (I am also seeing the failing TAP 
> tests), however, I have a concern. With a single server set up, when I 
> attempt to make a connection with target_session_attrs=read-write, I get the 
> message 
> psql: could not make a suitable connection to server "localhost:5432"
> Whereas, when I attempt to make a connection with 
> target_session_attrs=read-only, it is successful.
> 
> I might be missing something, but this seems somewhat counter-intuitive. I 
> would expect to specify read-write as target_session_attrs and successfully 
> connect to a server on which read and write operations are permitted. I see 
> this behavior implemented in src/interfaces/libpq/fe-connect.c
> Is there a reason to reject a connection to a primary server when I specify 
> 'read-write'? Is this intentional?
> 
> Hi Elvis,
> 
> Making an assumption about the intended functionality mentioned above, I 
> swapped the 'not' to the following lines of src/interfaces/libpq/fe-connect.c 
> ~ line 3005
> 
>   if (conn->target_session_attrs != NULL &&
>   ((strcmp(conn->target_session_attrs, 
> "read-write") == 0 && conn->session_read_only) ||
>(strcmp(conn->target_session_attrs, 
> "read-only") == 0 && !conn->session_read_only)))
> 
> I rebased and built with this change locally.
> The review below is based on the patch with that change.
> 
> Also, the following comment has what looks like a copy-paste error and the 
> first line should be deleted
> in src/backend/utils/misc/guc.c ~ line 10078
> assign_default_transaction_read_only
> 
> 
> +assign_default_transaction_read_only(bool newval, void *extra)
> ...
> + /*
> -  * We clamp manually-set values to at least 1MB.  Since
> +  * Also set the session read-only parameter.  We only need
> +  * to set the correct value in processes that have database
> +  * sessions, but there's no mechanism to know that there's
> 
> patch applies cleanly: yes
> installcheck: passed
> installcheck-world: passed
> feature works as expected: yes (details follow)
> 
> With two servers, one configured as the primary and one configured to run in 
> Hot Standby mode, I was able to observe that the value of session_read_only 
> changed after triggering failover once the standby server exited recovery
> 
> When attempting to connect to a primary server with 
> target_session_attrs=read-write, I was successful and when attempting to 
> connect with target_session_attrs=read-only, the connection was closed and 
> the expected message was produced

Based on the unaddressed questions raised in this thread, I’m marking this
patch Returned with Feedback.  Please re-submit a new version of the patch to a
future commitfest.

cheers ./daniel



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

Re: [HACKERS] [WIP] Zipfian distribution in pgbench

[Daniel Gustafsson]

> On 06 Sep 2017, at 08:42, Fabien COELHO  wrote:
> 
> Hello Alik,
> 
> Applies, compiles, works for me.
> 
> Some minor comments and suggestions.
> 
> Two typos:
> - "usinng" -> "using"
> - "a rejection method used" -> "a rejection method is used"
> 
> I'm not sure of "least_recently_used_i", this naming style is not used in 
> pgbench. "least_recently_used" would be ok.
> 
> "..nb_cells.. != ZIPF_CACHE_SIZE", ISTM that "<" is more logical,
> even if the result is the same?
> 
> I would put the parameter value check in getZipfianRand, so that if someone 
> reuse the function elsewhere the check is also performed. That would also 
> simplify a bit the already very large expression evaluation function.
> 
> When/if the pgbench tap test patch get through, coverage tests should
> be added.
> 
> Maybe the cache overflow could be counted, to allow for a possible warning 
> message in the final report?

Since this patch has been Waiting for author and no update on this patch has
been posted during the commitfest, it is Returned with feedback.  When you have
a new version of the patch, please re-submit to a future commitfest.

cheers ./daniel

-- 
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] Small patch for pg_basebackup argument parsing

[Daniel Gustafsson]

> On 18 Sep 2017, at 23:18, Pierre Ducroquet  wrote:
> 
> On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
>> Ryan Murphy  writes:
>>> Looked thru the diffs and it looks fine to me.
>>> Changing a lot of a integer/long arguments that were being read directly
>>> via atoi / atol to be read as strings first, to give better error
>>> handling.
>>> 
>>> This looks good to go to me.  Reviewing this as "Ready for Committer".
>>> Thanks for the patch, Pierre!
>> 
>> I took a quick look through this and had a few thoughts:
> 
> I agree with your suggestions, I will try to produce a new patch before the 
> end of the week.

This patch has been marked as Returned with Feedback as no new version has been
submitted.  Please re-submit the new version of this patch to the next commit-
fest when it’s ready.

cheers ./daniel

-- 
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] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 28 Sep 2017, at 14:55, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> 
> On Sun, 3 Sep 2017 22:47:10 +0200
> Daniel Gustafsson <dan...@yesql.se> wrote:
> 
> I have reviewed your latest patch. 
> 
> I can apply this to the master branch and build this successfully,
> and the behavior is as expected. 
> 
> However, here are some comments and suggestions.

Thanks for the review!  As I haven’t had time to address the comments for a new
versin of this patch, I’m closing this as Returned with Feedback and will
re-submit for the next commitfest.

cheers ./daniel

-- 
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] CLUSTER command progress monitor

[Daniel Gustafsson]

> On 12 Sep 2017, at 14:57, Tatsuro Yamada  wrote:
> 
> On 2017/09/12 21:20, Tatsuro Yamada wrote:
>> On 2017/09/11 23:38, Robert Haas wrote:
>>> On Sun, Sep 10, 2017 at 10:36 PM, Tatsuro Yamada
>>>  wrote:
 Thanks for the comment.
 
 As you know, CLUSTER command uses SEQ SCAN or INDEX SCAN as a scan method 
 by
 cost estimation. In the case of SEQ SCAN, these two phases not overlap.
 However, in INDEX SCAN, it overlaps. Therefore I created the phase of "scan
 heap and write new heap" when INDEX SCAN was selected.
 
 I agree that progress reporting for sort is difficult. So it only reports
 the phase ("sorting tuples") in the current design of progress monitor of
 cluster.
 It doesn't report counter of sort.
>>> 
>>> Doesn't that make it almost useless?  I would guess that scanning the
>>> heap and writing the new heap would ordinarily account for most of the
>>> runtime, or at least enough that you're going to want something more
>>> than just knowing that's the phase you're in.
>> 
>> Hmmm, Should I add a counter in tuplesort.c? (tuplesort_performsort())
>> I know that external merge sort takes a time than quick sort.
>> I'll try investigating how to get a counter from external merge sort 
>> processing.
>> Is this the right way?
>> 
>> 
> The patch is getting the value reported as heap_tuples_total from
> OldHeap->rd_rel->reltuples.  I think this is pointless: the user can
> see that value anyway if they wish.  The point of the progress
> counters is to expose things the user couldn't otherwise see.  It's
> also not necessarily accurate: it's only an estimate in the best case,
> and may be way off if the relation has recently be extended by a large
> amount.  I think it's pretty important that we try hard to only report
> values that are known to be accurate, because users hate (and mock)
> inaccurate progress reports.
 
 Do you mean to use the number of rows by using below calculation instead
 OldHeap->rd_rel->reltuples?
 
  estimate rows = physical table size / average row length
>>> 
>>> No, I mean don't report it at all.  The caller can do that calculation
>>> if they wish, without any help from the progress reporting machinery.
>> 
>> I see. I'll remove that column on next patch.
> 
> 
> I will summarize the current design and future corrections before sending
> the next patch.
> 
> 
> === Current design ===
> 
> CLUSTER command may use Index Scan or Seq Scan when scanning the heap.
> Depending on which one is chosen, the command will proceed in the
> following sequence of phases:
> 
>  * Scan method: Seq Scan
>1. scanning heap(*1)
>2. sorting tuples   (*2)
>3. writing new heap (*1)
>5. swapping relation files  (*2)
>6. rebuilding index (*2)
>7. performing final cleanup (*2)
> 
>  * Scan method: Index Scan
>4. scan heap and write new heap (*1)
>5. swapping relation files  (*2)
>6. rebuilding index (*2)
>7. performing final cleanup (*2)
> 
> VACUUM FULL command will proceed in the following sequence of phases:
> 
>1. scanning heap(*1)
>5. swapping relation files  (*2)
>6. rebuilding index (*2)
>7. performing final cleanup (*2)
> 
> (*1): increasing the value in heap_tuples_scanned column
> (*2): only shows the phase in the phase column
> 
> The view provides the information of CLUSTER command progress details as 
> follows
> # \d pg_stat_progress_cluster
>  View "pg_catalog.pg_stat_progress_cluster"
>  Column   |  Type   | Collation | Nullable | Default
> ---+-+---+--+-
> pid   | integer |   |  |
> datid | oid |   |  |
> datname   | name|   |  |
> relid | oid |   |  |
> command   | text|   |  |
> phase | text|   |  |
> scan_method   | text|   |  |
> scan_index_relid  | bigint  |   |  |
> heap_tuples_total | bigint  |   |  |
> heap_tuples_scanned   | bigint  |   |  |
> heap_tuples_vacuumed  | bigint  |   |  |
> heap_tuples_recently_dead | bigint  |   |  |
> 
> 
> === It will be changed on next patch ===
> 
> - Rename to pg_stat_progress_reolg from pg_stat_progress_cluster
> - Remove heap_tuples_total column from the view
> - Add a progress counter in the phase of "sorting tuples" (difficult?!)
> 
> 
> === My test case as a bonus ===
> 
> I share my test case of progress monitor.
> If someone wants to watch the current progress 

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

[Daniel Gustafsson]

> On 08 Apr 2017, at 16:14, David Steele  wrote:
> 
> On 3/22/17 6:20 AM, Etsuro Fujita wrote:
>> On 2017/02/22 19:57, Rushabh Lathia wrote:
>>> Marked this as Ready for Committer.
>> 
>> I noticed that this item in the CF app was incorrectly marked as
>> Committed.  This patch isn't committed, so I returned it to the previous
>> status.  I also rebased the patch.  Attached is a new version of the patch.
> 
> This submission has been moved to CF 2017-07.

This patch has been marked Ready for Committer in the current commitfest
without being committed or rejected.  Moving to the next commitfest, but since
it has bitrotted I’m moving it to Waiting for author.

cheers ./daniel

-- 
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] postgres_fdw: support parameterized foreign joins

[Daniel Gustafsson]

> On 11 Apr 2017, at 10:55, Etsuro Fujita  wrote:
> 
> On 2017/04/07 22:54, Arthur Zakirov wrote:
>> Marked the patch as "Ready for Commiter". But the patch should be
>> commited only after the patch [1].
> 
> Thanks for reviewing!  I'll continue to work on this for PG11.

This patch has been marked Ready for Committer in the current commitfest
without being committed or rejected.  Moving to the next commitfest, but since
it has bitrotted I’m moving it to Waiting for author.

cheers ./daniel

-- 
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] issue: record or row variable cannot be part of multiple-item INTO list

[Daniel Gustafsson]

> On 20 Sep 2017, at 01:05, David G. Johnston  
> wrote:
> 
> On Tue, Sep 19, 2017 at 11:29 AM, Tom Lane  > wrote:
> ​T​hat​ ​doesn't work today, and this patch doesn't fix it, but it does create
> enough confusion that we never would be able to fix it.
> 
> I'd be much happier if there were some notational difference
> between I-want-the-composite-variable-to-absorb-a-composite-column
> and I-want-the-composite-variable-to-absorb-N-scalar-columns.
> For backwards compatibility with what happens now, the latter would
> have to be the default.
> 
> ​So, using "()" syntax​
> 
> s,t: scalar text
> c,d: (text, text)
> 
> treat all numbers below as text; and the ((1,2),) as ("(1,2)",)
> 
> A. SELECT 1 INTO s; -- today 1, this patch is the same
> 
> B. SELECT 1, 2 INTO s; -- today 1, this patch is the same
> 
> C. SELECT 1, 2 INTO (s); -- ERROR syntax - scalars cannot be tagged with (), 
> this patch N/A
>  
> D. SELECT 1, 2 INTO s, t; -- today 1, 2, this patch is the same
> 
> E. SELECT 1, 2 INTO c; -- today (1,2), this patch is the same
> 
> F. SELECT 1, 2 INTO (c); --ERROR "1" cannot be converted to (text, text), 
> this patch N/A
> 
> 1. SELECT (1,2) INTO c; -- today ((1,2),); this patch is the same
> 
> 2. SELECT (1,2) INTO (c); -- (1,2) -- this patch N/A
> 
> 3. SELECT (1,2),(3,4) INTO c,d; -- ERROR syntax -- this patch gives [I 
> think...it can be made to give] (1,2),(3,4)
> 
> 4. SELECT (1,2),(3,4) INTO c,(d); -- ERROR syntax -- this patch N/A
> 
> 5. SELECT (1,2),(3,4) INTO (c),d; -- ERROR syntax -- this patch N/A
> 
> 6. SELECT (1,2),(3,4) INTO (c),(d); -- (1,2),(3,4) -- this patch N/A
> 
> !. SELECT 1, (2,3), 4 INTO s, (c), t -- 1, (2,3), 4 -- this patch N/A
> @. SELECT 1, 2, 3, 4 INTO s, (c), t -- ERROR "2" cannot be made into (text, 
> text) -- this patch N/A
> 
> IOW, this patch, if "c" is the only target (#1) and is composite pretend the 
> user wrote "INTO c.1, c.2" and assign each output column as a scalar in 
> one-by-one fashion.  If "c" is not the only target column (#3) assign a 
> single output column to it.  This maintains compatibility and clean syntax at 
> the cost of inconsistency.
> 
> The alternate, backward compatible, option introduces mandatory () in the 
> syntax for all composite columns in a multi-variable target (# 3-5 errors, #6 
> valid) while it changes the behavior if present on a single variable target 
> (#1 vs #2).
> 
> So, allowing #3 to work makes implementing #2 even more unappealing.  Making 
> only #2 and #6 work seems like a reasonable alternative position.
> 
> The last option is to fix #1 to return (1,2) - cleanly reporting an error if 
> possible, must like we just did with SRFs, and apply the patch thus gaining 
> both consistency and a clean syntax at the expense of limited backward 
> incompatibility.
> 
> Arrays not considered; single-column composites might end up looking like 
> scalars when presented to a (c) target.
> 
> Hope this helps someone besides me understand the problem space.

Based on the discussion in this thread I’ve moved this patch to the next CF
with the new status Waiting for author, as it seems to need a revised version.

cheers ./daniel



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

Re: [HACKERS] parallelize queries containing initplans

[Daniel Gustafsson]

> On 15 Sep 2017, at 04:45, Amit Kapila  wrote:
> 
> On Thu, Aug 31, 2017 at 11:23 AM, Amit Kapila  wrote:
>> On Mon, Aug 21, 2017 at 2:40 PM, Amit Kapila  wrote:
>>> On Mon, Aug 21, 2017 at 1:44 PM, Haribabu Kommi
>>>  wrote:
 
 
 Thanks for adding more details. It is easy to understand.
 
 I marked the patch as ready for committer in the commitfest.
>> 
>> Rebased the patch.  The output of test case added by the patch is also
>> slightly changed because of the recent commit
>> 7df2c1f8daeb361133ac8bdeaf59ceb0484e315a.  I have verified that the
>> new test result is as expected.
> 
> The latest patch again needs to be rebased.  Find rebased patch
> attached with this email.

Moved to next commitfest, but changed to Waiting for author since it no longer
applies cleanly.

cheers ./daniel

-- 
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] Vacuum: allow usage of more than 1GB of work mem

[Daniel Gustafsson]

> On 18 Aug 2017, at 13:39, Claudio Freire  wrote:
> 
> On Fri, Apr 7, 2017 at 10:51 PM, Claudio Freire  
> wrote:
>> Indeed they do, and that's what motivated this patch. But I'd need
>> TB-sized tables to set up something like that. I don't have the
>> hardware or time available to do that (vacuum on bloated TB-sized
>> tables can take days in my experience). Scale 4000 is as big as I can
>> get without running out of space for the tests in my test hardware.
>> 
>> If anybody else has the ability, I'd be thankful if they did test it
>> under those conditions, but I cannot. I think Anastasia's test is
>> closer to such a test, that's probably why it shows a bigger
>> improvement in total elapsed time.
>> 
>> Our production database could possibly be used, but it can take about
>> a week to clone it, upgrade it (it's 9.5 currently), and run the
>> relevant vacuum.
> 
> It looks like I won't be able to do that test with a production
> snapshot anytime soon.
> 
> Getting approval for the budget required to do that looks like it's
> going to take far longer than I thought.
> 
> Regardless of that, I think the patch can move forward. I'm still
> planning to do the test at some point, but this patch shouldn't block
> on it.

This patch has been marked Ready for committer after review, but wasn’t
committed in the current commitfest so it will be moved to the next.  Since it
no longer applies cleanly, it’s being reset to Waiting for author though.

cheers ./daniel

-- 
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] Crash on promotion when recovery.conf is renamed

[Daniel Gustafsson]

> On 02 Sep 2017, at 14:17, Michael Paquier  wrote:
> 
> On Fri, Sep 1, 2017 at 7:17 PM, Thomas Munro
>  wrote:
>> On Sat, Aug 26, 2017 at 7:32 AM, Robert Haas  wrote:
>>> On Sat, Apr 8, 2017 at 10:05 AM, David Steele  wrote:
 On 3/28/17 1:21 AM, Tsunakawa, Takayuki wrote:
> Thanks, marked as ready for committer, as the code is good and Alexander 
> reported the test success.
 
 This bug has been moved to CF 2017-07.
>>> 
>>> This bug fix has been pending in "Ready for Committer" state for about
>>> 4.5 months.  Three committers (Magnus, Heikki, Tom) have contributed
>>> to the thread to date.  Maybe one of them would like to commit this?
>> 
>> In the meantime its bits have begun to rot.  Michael, could you please 
>> rebase?
> 
> Thanks for the reminder, Thomas. The 2PC code triggered during
> recovery has been largely reworked in PG10, explaining the conflicts.
> As this has been some time since I touched this patch, I had again a
> look at its logic and could not find any problems around it. So
> attached are a rebased versions for both 0001 and 0002, I have updated
> the messages as well to be more in-line with what is in HEAD for
> corrupted entries.

I’ve moved this to the next CF, but since this no longer applies cleanly I’ve
reset it to Waiting for author.

cheers ./daniel

-- 
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] Statement-level rollback

[Daniel Gustafsson]

> On 15 Sep 2017, at 16:19, Daniel Gustafsson <dan...@yesql.se> wrote:
> 
>> On 01 Sep 2017, at 13:44, Simon Riggs <si...@2ndquadrant.com> wrote:
>> 
>> On 14 August 2017 at 23:58, Peter Eisentraut
>> <peter.eisentr...@2ndquadrant.com> wrote:
>>> On 2/28/17 02:39, Tsunakawa, Takayuki wrote:
>>>> The code for stored functions is not written yet, but I'd like your 
>>>> feedback for the specification and design based on the current patch.  
>>>> I'll add this patch to CommitFest 2017-3.
>>> 
>>> This patch needs to be rebased for the upcoming commit fest.
>> 
>> I'm willing to review this if the patch is going to be actively worked on.
> 
> This sounds like a too good offer to pass up on, can we expect a rebased patch
> for the commitfest?

Since this patch was Waiting for author during the entire commitfest without
updates, I’m marking it Returned with Feedback.  When a new version is ready it
can be re-submitted to the then open commitfest.

cheers ./daniel

-- 
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] ANALYZE command progress checker

[Daniel Gustafsson]

> On 05 Apr 2017, at 03:17, Masahiko Sawada  wrote:
> 
> On Wed, Apr 5, 2017 at 1:49 AM, Robert Haas  wrote:
>> On Tue, Apr 4, 2017 at 4:57 AM, Amit Langote
>>  wrote:
>>> Hmm, you're right.  It could be counted with a separate variable
>>> initialized to 0 and incremented every time we decide to add a row to the
>>> final set of sampled rows, although different implementations of
>>> AcquireSampleRowsFunc have different ways of deciding if a given row will
>>> be part of the final set of sampled rows.
>>> 
>>> On the other hand, if we decide to count progress in terms of blocks as
>>> you suggested afraid, I'm afraid that FDWs won't be able to report the
>>> progress.
>> 
>> I think it may be time to push this patch out to v11.  It was
>> submitted one day before the start of the last CommitFest, the design
>> wasn't really right, and it's not clear even now that we know what the
>> right design is.  And we're pretty much out of time.
> 
> +1
> We're encountering the design issue and it takes more time to find out
> right design including FDWs support.

I’m marking this patch Returned with Feedback since it has been Waiting for
author during the commitfest without updates.

cheers ./daniel

-- 
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: faster version of tpcb-like transaction

[Daniel Gustafsson]

> On 27 Aug 2017, at 08:37, Fabien COELHO  wrote:
> 
> 
> About the patch:
> 
> I'm generally in favor of providing more options to pgbench, especially if it 
> can give optimization ideas to the performance conscious user.
> 
> I think that the name should be "tpcb-like-plfunc": the script does not 
> implement tpcb per spec, and such a function could be written in another 
> language with some performance benefit, or not.
> 
> Maybe that mean to relax the prefix condition to "take the first matching 
> name" when prefix are used.
> 
> If you are reimplementing the transaction anyway, you could consider using 
> UPDATE RETURNING instead of SELECT to get the balance. On the other hand the 
> doc says that the "steps" are put in a PL function, so maybe it should 
> reflect the original script.
> 
> I'm surprised by:
> 
>  "select * from pgbench_transaction(:aid, :bid, :tid, :delta);\n"
> 
> Why not simply:
> 
>"select pgbench_transaction(:aid, :bid, :tid, :delta);\n"
> 
> I would suggest to use a more precise function name, in case other functions 
> are thought of. Maybe "pgbench_tpcb_like_plfunc".
> 
> I would suggest to indent better the PL/function and put keywords and types 
> in capital, and add explicitely the properties of the function (eg STRICT, 
> VOLATILE?).
> 
> There is a spurious space at the end of the executeStatement call line.
> 
> The patch potentially interacts with other patches in the long and slow 
> queue...
> 
> As usual with pgbench there are no regression tests.

This patch has been Waiting for author during the commitfest without updates,
moving to Returned with feedback.

cheers ./daniel



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

Re: [HACKERS] PATCH: psql show index with type info

[Daniel Gustafsson]

> On 13 Sep 2017, at 01:24, Daniel Gustafsson <dan...@yesql.se> wrote:
> 
>> On 18 Apr 2017, at 05:13, Amos Bird <amosb...@gmail.com> wrote:
>> 
>> Ah, thanks for the suggestions. I'll revise this patch soon :)
> 
> Have you had a chance to revise the patch to address the review comments such
> that the patch can move forward during this Commitfest?

Since the patch was Waiting for author without being updated during the
commitfest, it has been Returned with Feedback.

cheers ./daniel

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

Re: [HACKERS] [PATCH] Off-by-one error in logical slot resource retention

[Daniel Gustafsson]

> On 15 Sep 2017, at 13:26, Daniel Gustafsson <dan...@yesql.se> wrote:
> 
>> On 01 Sep 2017, at 14:28, Aleksander Alekseev <a.aleks...@postgrespro.ru> 
>> wrote:
>> 
>> The following review has been posted through the commitfest application:
>> make installcheck-world:  not tested
>> Implements feature:   not tested
>> Spec compliant:   not tested
>> Documentation:not tested
>> 
>> Hi Craig,
>> 
>> I'm afraid patches 0002 and 0003 don't apply anymore. Could you please 
>> resolve the conflicts?
>> 
>> The new status of this patch is: Waiting on Author
> 
> Have you had a chance to look at rebasing this patch so we can get it further
> in the process?

Since this patch has been in Waiting for Author state for the duration of the
commitfest without moving, I’m marking it Returned with Feedback.  If there is
still interest in pursuing this patch, please re-submit it to the next
commitfest with the comments addressed.

cheers ./daniel

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

Re: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

[Daniel Gustafsson]

> On 31 Aug 2017, at 21:49, Peter Eisentraut  
> wrote:
> 
> On 5/30/17 23:10, Peter Eisentraut wrote:
>> Here is a proposed solution that splits bgw_name into bgw_type and
>> bgw_name_extra.  bgw_type shows up in pg_stat_activity.backend_type.
>> Uses of application_name are removed, because they are no longer
>> necessary to identity the process type.
> 
> Updated patch incorporating the feedback.  I have kept bgw_name as it
> was and just added bgw_type completely independently.

The patch applies with minor fuzz, compiles without introduced warnings and
work the way it says on the tin.  The utility of the proposed functionality is
a clear win so +1 on getting that in.

> One open question is how to treat a missing (empty) bgw_type.  I
> currently fill in bgw_name as a fallback.  We could also treat it as an
> error or a warning as a transition measure.

Warnings tend to stick around forever in my experience.  An error would work as
a transition measure, but it seems a hammer too far.

Falling back on bgw_name solves there being data with the risk that some
workers will have a wonky type displayed, and those are the ones that of course
will never get updated.  Either going with NULL as Michael suggested elsewhere
in this thread (my preference as well) or a default string along the lines of
“Unknown type” would probably carry more incentive to update.

A few random comments on the patch:

Regarding the following hunk:

+   process listing, for example.  bgw_name on the
+   other hand can contain additional information about the specific process.
+   (Typically, the string for bgw_name will contain
+   the string for bgw_type somehow, but that is not
+   strictly required.)

This reads a bit too (oddly) detailed to me, I would’ve phrased it as something
along the lines of:

"Typically, the string for bgw_name will contain the type somehow, but that
 is not strictly required.”

I find omitting “background worker” in the following hunk is leaving out
valuable information for bgworkers with badly configured types, but it’s
definitely a matter of taste rather than a straight-up patch critique:

-errmsg("terminating background worker \"%s\" due to administrator 
command",
-   MyBgworkerEntry->bgw_name)));
+errmsg("terminating %s due to administrator command",
+   MyBgworkerEntry->bgw_type)));

Updating the status to “Ready for committer” with the discussion around missing
bgw_type left as a decision point for a committer.

cheers ./daniel

-- 
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] Repetitive code in RI triggers

[Daniel Gustafsson]

> On 26 Sep 2017, at 10:51, Maksim Milyutin <milyuti...@gmail.com> wrote:
> 
> On 19.09.2017 11:09, Daniel Gustafsson wrote:
> 
>> Removing reviewer Maksim Milyutin from patch entry due to inactivity and
>> community account email bouncing.  Maksim: if you are indeed reviewing this
>> patch, then please update the community account and re-add to the patch 
>> entry.
>> 
>> cheers ./daniel
> 
> Daniel, thanks for noticing. I have updated my account and re-added to the 
> patch entry.

Great, thanks!

> Ildar, your patch is conflicting with the current HEAD of master branch, 
> please update it.

I’ve changed status to Waiting on Author based on this.

cheers ./daniel

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

[HACKERS] Commitfest 201709 update

[Daniel Gustafsson]

Two thirds of the commitfest has now passed, and while a lot of patches have
been closed, there is still lots to do.  As of now, 48% of the entries are
either in “Needs review” or “Waiting on Author”.  Let’s try to reduce that
number further!

A lot of you authors/reviewers have responded to my nagging/pleading emails
with picking up work, and we are all grateful for your efforts.  Those of you
who are not reviewers in this commitfest, please consider picking up a patch to
review.  PostgreSQL runs on the collective effort put in by the community, and
all contributions are welcome.

Thank you all for your contributions to PostgreSQL!

Sincerely

Daniel Gustafsson
CFM201709

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

Re: [HACKERS] PoC: full merge join on comparison clause

[Daniel Gustafsson]

> On 19 Sep 2017, at 15:18, Ashutosh Bapat  
> wrote:
> 
> On Fri, Aug 25, 2017 at 10:11 PM, Alexander Kuzmenkov
>  wrote:
>> Here is a new version of the patch, rebased to 749c7c41 and with some
>> cosmetic changes.
>> 
> 
> I looked at this patch briefly. This is a useful feature. This isn't a
> design level review of the patch. I may get back to that later. But
> here are some assorted comments

> ..

Looking forward to further review on this patch, but based on this feedback I’m
moving this to Waiting for author.

cheers ./daniel

-- 
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] Repetitive code in RI triggers

[Daniel Gustafsson]

> On 11 Apr 2017, at 03:41, Peter Eisentraut  
> wrote:
> 
> On 4/10/17 11:55, Ildar Musin wrote:
>> I was looking through the RI triggers code recently and noticed a few 
>> almost identical functions, e.g. ri_restrict_upd() and 
>> ri_restrict_del(). The following patch is an attempt to reduce some of 
>> repetitive code.
> 
> That looks like something worth pursuing.  Please add it to the next
> commit fest.

Removing reviewer Maksim Milyutin from patch entry due to inactivity and
community account email bouncing.  Maksim: if you are indeed reviewing this
patch, then please update the community account and re-add to the patch entry.

cheers ./daniel

-- 
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] Other formats in pset like markdown, rst, mediawiki

[Daniel Gustafsson]

> On 18 Sep 2017, at 15:31, Jan Michálek  wrote:
> 
> I have`t any new version recently, because i was waiting for new version. But 
> I will have some time on this in next months. How many time I have to next 
> freeze?

The next commitfest runs from 2017-11-01 to 2017-11-30, I will move your patch
there.

cheers ./daniel

-- 
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] Should we cacheline align PGXACT?

[Daniel Gustafsson]

> On 16 Sep 2017, at 01:51, Alexander Korotkov <a.korot...@postgrespro.ru> 
> wrote:
> 
> On Tue, Sep 5, 2017 at 2:47 PM, Daniel Gustafsson <dan...@yesql.se 
> <mailto:dan...@yesql.se>> wrote:
> > On 04 Apr 2017, at 14:58, David Steele <da...@pgmasters.net 
> > <mailto:da...@pgmasters.net>> wrote:
> >
> > On 4/4/17 8:55 AM, Alexander Korotkov wrote:
> >> On Mon, Apr 3, 2017 at 9:58 PM, Andres Freund <and...@anarazel.de 
> >> <mailto:and...@anarazel.de>
> >>
> >>I'm inclined to push this to the next CF, it seems we need a lot more
> >>benchmarking here.
> >>
> >> No objections.
> >
> > This submission has been moved to CF 2017-07.
> 
> This CF has now started (well, 201709 but that’s what was meant in above), can
> we reach closure on this patch in this CF?
> 
> During previous commitfest I come to doubts if this patch is really needed 
> when same effect could be achieved by another (perhaps random) change of 
> alignment.  The thing I can do now is to retry my benchmark on current master 
> and check what effect this patch has now.

Considering this I’ll mark this as Waiting on Author, in case you come to
conclusion that another patch is required then we’ll bump to a return status.

cheers ./daniel 

-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Daniel Gustafsson]

> On 15 Sep 2017, at 17:19, Robert Haas <robertmh...@gmail.com> wrote:
> 
> On Fri, Sep 15, 2017 at 10:15 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
>> Have you had a chance to look at this such that we can expect a rebased 
>> version
>> of this patch during the commitfest?
> 
> Frankly, I think things where there was a ping multiple weeks before
> the CommitFest started and no rebase before it started should be
> regarded as untimely submissions, and summarily marked Returned with
> Feedback.  The CommitFest is supposed to be a time to get things that
> are ready before it starts committed before it ends.  Some amount of
> back-and-forth during the CF is of course to be expected, but we don't
> even really have enough bandwidth to deal with the patches that are
> being timely updated, never mind the ones that aren’t.

I don’t necessarily disagree with this, and especially not the part about
bandwidth which is absolutely correct.

What has happened a lot however is that these patches have been moved to the
next CF, and possibly the next from there.  In that scenario, if we can get the
patches rebased now they wont be in a worse state when pushed to the next
compared to if they bitrot further.  That being said, perhaps we should move
closer to a model like what you describe, but thats for another thread to
discuss rather than threadjacking this one more IMO.

cheers ./daniel

-- 
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] Renaming PG_GETARG functions (was Re: PG_GETARG_GISTENTRY?)

[Daniel Gustafsson]

> On 12 Sep 2017, at 22:07, Tom Lane  wrote:
> 
> [ changing subject line to possibly draw more attention ]
> 
> Mark Dilger  writes:
>>> On Apr 5, 2017, at 9:23 AM, Tom Lane  wrote:
>>> In short, if you are supposed to write
>>> FOO  *val = PG_GETARG_FOO(n);
>>> then the macro designer blew it, because the name implies that it
>>> returns FOO, not pointer to FOO.  This should be
>>> FOO  *val = PG_GETARG_FOO_P(n);
> 
>> I have written a patch to fix these macro definitions across src/ and
>> contrib/.
> 
> So to summarize, this patch proposes to rename some DatumGetFoo,
> PG_GETARG_FOO, and PG_RETURN_FOO macros for these datatypes:
> 
> NDBOX (contrib/cube)
> HSTORE
> LTREE and other contrib/ltree types
> 
> PG_GETARG_ANY_ARRAY (and there are some related macros it maybe should
> have touched, like PG_RETURN_EXPANDED_ARRAY)
> 
> JSONB
> 
> RANGE
> 
> The contrib types don't seem like much of a problem, but I wonder
> whether anyone feels that rationalizing the names for array, JSON,
> or range-type macros will break too much code.
> 
> One option if we do feel that way is that we could provide the
> old names as alternatives, thus not breaking external modules.
> But that seems like it's sabotaging the basic goal of improving
> consistency of naming.
> 
> If there are not objections, I plan to push forward with this.

Judging by this post, I’m updating this to “Ready for Committer”.

cheers ./daniel

-- 
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] Statement-level rollback

[Daniel Gustafsson]

> On 01 Sep 2017, at 13:44, Simon Riggs  wrote:
> 
> On 14 August 2017 at 23:58, Peter Eisentraut
>  wrote:
>> On 2/28/17 02:39, Tsunakawa, Takayuki wrote:
>>> The code for stored functions is not written yet, but I'd like your 
>>> feedback for the specification and design based on the current patch.  I'll 
>>> add this patch to CommitFest 2017-3.
>> 
>> This patch needs to be rebased for the upcoming commit fest.
> 
> I'm willing to review this if the patch is going to be actively worked on.

This sounds like a too good offer to pass up on, can we expect a rebased patch
for the commitfest?

cheers ./daniel

-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Daniel Gustafsson]

> On 15 Aug 2017, at 01:00, Peter Eisentraut  
> wrote:
> 
> On 4/3/17 22:00, Etsuro Fujita wrote:
>> On 2017/04/04 3:21, Andres Freund wrote:
>>> On 2017-02-28 21:45:22 +0900, Etsuro Fujita wrote:
 Here is a patch for $subject.
>>> 
>>> This is a nontrivial patch, submitted just before the start of the last
>>> CF for postgres 10.  Therefore I think we should move this to the next
>>> CF.
>> 
>> Honestly, I'm not satisfied with this patch and I think it would need 
>> more work.  Moved to the next CF.
> 
> This patch needs to be rebased for the upcoming commit fest.

Have you had a chance to look at this such that we can expect a rebased version
of this patch during the commitfest?

cheers ./daniel

-- 
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] SQL/JSON in PostgreSQL

[Daniel Gustafsson]

> On 15 Aug 2017, at 04:30, Peter Eisentraut  
> wrote:
> 
> On 3/15/17 11:56, David Steele wrote:
>>> This patch has been moved to CF 2017-07.
>> 
>> I did not manage to move this patch when I said had.  It is now moved.
> 
> Unsurprisingly, this patch needs a major rebase.

Can we expect a rebased version of this patch for this commitfest?  Since it’s
a rather large feature it would be good to get it in as early as we can in the
process.

cheers ./daniel

-- 
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] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

[Daniel Gustafsson]

> On 05 Sep 2017, at 10:44, Haribabu Kommi  wrote:
> 
> On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja  > wrote:
> On Fri, Jul 1, 2016 at 2:12 AM, I wrote:
> Currently the tuple returned by INSTEAD OF triggers on DELETEs is only used 
> to determine whether to pretend that the DELETE happened or not, which is 
> often not helpful enough; for example, the actual tuple might have been 
> concurrently UPDATEd by another transaction and one or more of the columns 
> now hold values different from those in the planSlot tuple. Attached is an 
> example case which is impossible to properly implement under the current 
> behavior.  For what it's worth, the current behavior seems to be an accident; 
> before INSTEAD OF triggers either the tuple was already locked (in case of 
> BEFORE triggers) or the actual pre-DELETE version of the tuple was fetched 
> from the heap.
> 
> So I'm suggesting to change this behavior and allow INSTEAD OF DELETE 
> triggers to modify the OLD tuple and use that for RETURNING instead of 
> returning the tuple in planSlot.  Attached is a WIP patch implementing that.
> 
> Is there any reason why we wouldn't want to change the current behavior?
> 
> Since nobody seems to have came up with a reason, here's a patch for that 
> with test cases and some documentation changes.  I'll also be adding this to 
> the open commit fest, as is customary.
> 
> Thanks for the patch. This patch improves the DELETE returning
> clause with the actual row.
> 
> Compilation and tests are passed. I have some review comments.
> 
> ! that was provided.  Likewise, for DELETE operations the
> ! OLD variable can be modified before returning it, and
> ! the changes will be reflected in the output data.
> 
> The above explanation is not very clear, how about the following?
> 
> Likewise, for DELETE operations the trigger may 
> modify the OLD row before returning it, and the
> change will be reflected in the output data of DELETE RETURNING.
> 
> 
> ! TupleTableSlot *
>   ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
> !  HeapTuple trigtuple, TupleTableSlot 
> *slot)
> 
> ! oldtuple = ExecMaterializeSlot(slot); --nodeModifyTable.c
> 
> The trigtuple is part of the slot anyway, I feel there is no need to pass
> the trigtuple seperately. The tuple can be formed internaly by Materializing
> slot.
> 
> Or
> 
> Don't materialize the slot before the ExecIRDeleteTriggers function
> call.
> 
> ! /*
> !  * Return the modified tuple using the es_trig_tuple_slot.  We 
> assume
> !  * the tuple was allocated in per-tuple memory context, and 
> therefore
> !  * will go away by itself. The tuple table slot should not try 
> to
> !  * clear it.
> !  */
> ! TupleTableSlot *newslot = estate->es_trig_tuple_slot;
> 
> The input slot that is passed to the function ExecIRDeleteTriggers
> is same as estate->es_trig_tuple_slot. And also the tuple descriptor
> is same. Instead of using the newslot, directly use the slot is fine.
> 
> 
> + /* trigger might have changed tuple */
> + oldtuple = ExecMaterializeSlot(slot);
> 
> 
> + if (resultRelInfo->ri_TrigDesc &&
> + resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
> + return ExecProcessReturning(resultRelInfo, slot, 
> planSlot);
> 
> 
> Views cannot have before/after triggers, Once the call enters into
> Instead of triggers flow, the oldtuple is used to frame the slot, if the
> returning clause is present. But in case of instead of triggers, the call
> is returned early as above and the framed old tuple is not used.
> 
> Either change the logic of returning for instead of triggers, or remove
> the generation of oldtuple after instead triggers call execution.

Have you had a chance to work on this patch to address the above review?

cheers ./daniel

-- 
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] VACUUM and ANALYZE disagreeing on what reltuples means

[Daniel Gustafsson]

> On 06 Sep 2017, at 09:45, Haribabu Kommi  wrote:
> 
> On Tue, Jul 25, 2017 at 9:33 PM, Tomas Vondra  > wrote:
> On 7/25/17 12:55 AM, Tom Lane wrote:
> Tomas Vondra  > writes:
> It seems to me that VACUUM and ANALYZE somewhat disagree on what
> exactly reltuples means. VACUUM seems to be thinking that reltuples
> = live + dead while ANALYZE apparently believes that reltuples =
> live
> 
> The question is - which of the reltuples definitions is the right
> one? I've always assumed that "reltuples = live + dead" but perhaps
> not?
> 
> I think the planner basically assumes that reltuples is the live
> tuple count, so maybe we'd better change VACUUM to get in step.
> 
> Attached is a patch that (I think) does just that. The disagreement was 
> caused by VACUUM treating recently dead tuples as live, while ANALYZE treats 
> both of those as dead.
> 
> At first I was worried that this will negatively affect plans in the 
> long-running transaction, as it will get underestimates (due to reltuples not 
> including rows it can see). But that's a problem we already have anyway, you 
> just need to run ANALYZE in the other session.
> 
> Thanks for the patch.
> From the mail, I understand that this patch tries to improve the
> reltuples value update in the catalog table by the vacuum command
> to consider the proper visible tuples similar like analyze command.
> 
> - 
>  num_tuples);
> + 
>  num_tuples - nkeep);
> 
> With the above correction, there is a problem in reporting the number
> of live tuples to the stats.
> 
> postgres=# select reltuples, n_live_tup, n_dead_tup
>   from pg_stat_user_tables join pg_class using (relname)
>  where relname = 't';
>  reltuples | n_live_tup | n_dead_tup 
> ---++
> 899818 | 799636 | 100182
> (1 row)
> 
> 
> The live tuples data value is again decremented with dead tuples
> value before sending them to stats in function lazy_vacuum_rel(),
> 
>   /* report results to the stats collector, too */
>   new_live_tuples = new_rel_tuples - vacrelstats->new_dead_tuples;
> 
> The fix needs a correction here also. Or change the correction in 
> lazy_vacuum_rel() function itself before updating catalog table similar
> like stats.

This patch is marked Waiting for Author, have you had a chance to look at this
to address the comments in the above review?

cheers ./daniel

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

Re: [HACKERS] [PATCH] Off-by-one error in logical slot resource retention

[Daniel Gustafsson]

> On 01 Sep 2017, at 14:28, Aleksander Alekseev  
> wrote:
> 
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:not tested
> 
> Hi Craig,
> 
> I'm afraid patches 0002 and 0003 don't apply anymore. Could you please 
> resolve the conflicts?
> 
> The new status of this patch is: Waiting on Author

Have you had a chance to look at rebasing this patch so we can get it further
in the process?

cheers ./daniel

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

Re: [HACKERS] Patch: add --if-exists to pg_recvlogical

[Daniel Gustafsson]

> On 09 Sep 2017, at 06:05, Thomas Munro  wrote:
> 
> On Sat, Sep 2, 2017 at 5:22 AM, Peter Eisentraut
>  wrote:
>>  
>> +  --if-exists
>> +  
>> +   
>> +Do not error out when --drop-slot or
>> --start are
>> +specified and a slot with the specified name does not exist.
>> +   
>> +  
>> + 
>> 
>> I understand the --drop-slot part.  But I don't understand what it means
>> to ignore a missing replication slot when running --start.
> 
> Also "--start" breaks the documentation build (missing
> slash on the closing tag).

This patch is "Waiting for Author” due to the above review comments from Peter
and Thomas.  Do you think you will have time to address these shortly so we can
move this patch further in the process?

cheers ./daniel

-- 
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] Bug with pg_basebackup and 'shared' tablespace

[Daniel Gustafsson]

> On 15 May 2017, at 07:26, Michael Paquier  wrote:
> 
> On Tue, May 2, 2017 at 2:28 AM, Pierre Ducroquet  wrote:
> 
>> I will submit this patch in the current commit fest.
> 
> I have not spotted any flaws in the refactored logic.

This patch no longer applies, could you take a look at it and submit a new
version rebased on top of HEAD?

cheers ./daniel

-- 
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] Patches that don't apply or don't compile: 2017-09-12

[Daniel Gustafsson]

> On 13 Sep 2017, at 11:49, Aleksander Alekseev  
> wrote:
> 
> Hi Tomas,
> 
> I appreciate your feedback, although it doesn't seem to be completely
> fair.

I would like to stress one thing (and I am speaking only for myself here), this
has been feedback and not criticism.  Your (and everyone involved in this)
initiative is great and automation will no doubt make the CF process better.
We just need to finetune it a little to make it work for, as well as with, the
community.

cheers ./daniel

-- 
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] Small patch for pg_basebackup argument parsing

[Daniel Gustafsson]

> On 05 Jul 2017, at 08:32, Michael Paquier  wrote:
> 
> On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy  wrote:
>> I tried to apply your patch to test it (though reading Robert's last comment 
>> it seems we wish to have it adjusted before committing)... but in any case I 
>> was not able to apply your patch to the tip of the master branch (my git 
>> apply failed).  I'm setting this to Waiting On Author for a new patch, and I 
>> also agree with Robert that the test can be simpler and can go in the other 
>> order.  If you don't have time to make another patch, let me know, I may be 
>> able to make one.
> 
> git is unhappy even if forcibly applied with patch -p1. You should
> check for whitespaces at the same time:
> $ git diff --check
> src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
> +char   *strtol_endptr = NULL
> And there are more than this one.

Like Michael said above, this patch no longer applies and have some whitespace
issues.  The conflicts in applying are quite trivial though, so it should be
easy to create a new version.  Do you plan to work on this during this
Commitfest so we can move this patch forward?

cheers ./daniel

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

Re: [HACKERS] Replication status in logical replication

[Daniel Gustafsson]

> On 30 May 2017, at 19:55, Peter Eisentraut  
> wrote:
> 
> On 5/29/17 22:56, Noah Misch wrote:
>> On Fri, May 19, 2017 at 11:33:48AM +0900, Masahiko Sawada wrote:
>>> On Wed, Apr 12, 2017 at 5:31 AM, Simon Riggs  wrote:
 Looks like a bug that we should fix in PG10, with backpatch to 9.4 (or
 as far as it goes).
 
 Objections to commit?
 
>>> 
>>> Seems we still have this issue. Any update or comment on this? Barring
>>> any objections, I'll add this to the open item so it doesn't get
>>> missed.
>> 
>> [Action required within three days.  This is a generic notification.]
>> 
>> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> v10 open item, please let us know.
> 
> I would ask Simon to go ahead with this patch if he feels comfortable
> with it.
> 
> I'm disclaiming this open item, since it's an existing bug from previous
> releases (and I have other open items to focus on).

I’m not entirely sure why this was flagged as "Waiting for Author” by the
automatic run, the patch applies for me and builds so resetting back to “Needs
review”.

Simon: do you think you will have time to look at this patch in this CF?

cheers ./daniel

-- 
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] Faster methods for getting SPI results

[Daniel Gustafsson]

> On 12 Sep 2017, at 23:00, Tom Lane  wrote:
> 
> Chapman Flack  writes:
>> On 09/12/2017 03:41 PM, Tom Lane wrote:
>>> So the conclusion at the end of the last commitfest was that this patch
>>> should be marked Returned With Feedback, and no new work appears to have
>>> been done on it since then.  Why is it in this fest at all?  There
>>> certainly doesn't seem to be any reason to review it again.
> 
>> I'm not sure how to read the history of the CF entry. Could it
>> have rolled over to 2017-09 by default if its status was simply
>> never changed to Returned with Feedback as intended in the last
>> one? The history doesn't seem to show anything since 2017-03-19.
> 
> Maybe, or whoever was closing out the last CF didn't notice Andres'
> recommendation to mark it RWF.

It doesn’t seem to have been moved to this CF but was actually created here in
the first place.  Reading this thread it seems like there is clear concensus on
the status though so changing to RWF.

cheers ./daniel

-- 
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] plpgsql - additional extra checks

[Daniel Gustafsson]

> On 08 Apr 2017, at 15:46, David Steele  wrote:
> 
>> On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
>>> On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby >> > wrote:
>>> 
>>>On 1/11/17 5:54 AM, Pavel Stehule wrote:
>>> 
>>>+too_many_rows
>>>+
>>>+ 
>>>+  When result is assigned to a variable by
>>>INTO clause,
>>>+  checks if query returns more than one row. In this case
>>>the assignment
>>>+  is not deterministic usually - and it can be signal some
>>>issues in design.
>>> 
>>> 
>>>Shouldn't this also apply to
>>> 
>>>var := blah FROM some_table WHERE ...;
>>> 
>>>?
>>> 
>>>AIUI that's one of the beefs the plpgsql2 project has.
>>> 
>>> 
>>> No, not at all.  That syntax is undocumented and only works because
>>> PL/PgSQL is a hack internally.  We don't use it, and frankly I don't
>>> think anyone should.
> 
> This submission has been moved to CF 2017-07.

This patch was automatically marked as “Waiting for author” since it needs to
be updated with the macro changes in 2cd70845240087da205695baedab6412342d1dbe
to compile.  Changing to using TupleDescAttr(); makes it compile again.  Can
you submit an updated version with that fix Pavel?

Stephen, you signed up to review this patch in the previous Commitfest, do you
still intend to work on this?

cheers ./daniel

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

Re: [HACKERS] PATCH: psql show index with type info

[Daniel Gustafsson]

> On 18 Apr 2017, at 05:13, Amos Bird  wrote:
> 
> Ah, thanks for the suggestions. I'll revise this patch soon :)

Have you had a chance to revise the patch to address the review comments such
that the patch can move forward during this Commitfest?

cheers ./daniel


-- 
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] Other formats in pset like markdown, rst, mediawiki

[Daniel Gustafsson]

> On 08 May 2017, at 12:02, Fabien COELHO  
> wrote:
> 
> Hello Jan,
> 
> Please give a number to submitted patches. I think that this was v3.
> 
> The patch does NOT fix various issues I pointed out in my previous review:
> 
> - tabs introduced in "doc/src/sgml/ref/psql-ref.sgml"
> - too long help line in "src/bin/psql/help.c"
> - spurious space after a comma in "src/fe_utils/print.c"
>   and possibly elsewhere.
> 
> On Sun, 23 Apr 2017, Jan Michálek wrote:
> 
 Markdown include characters/sequences which are interpreted as markers:
 _Italic_, **Bold**, *** => horizontal rules, > block quote... `inline
 code`... If they are interpreted within a table cell then probably they
 should be escaped somehow.
>> 
>> I have treated "_*|<>"
> 
> Probably not enough, see below. Note the escaping chars should also be 
> escaped.
> 
>>> I`m able to sanitize characters, but complex sequences will be problem. I
>>> will look on this, but I don`t know, if I`m able to do this.
> 
> I do not know whether only those are necessary. Have you checked? Guessing is 
> probably not the right approach.
> 
> 
> Concerning MARKDOWN, and according to the following source about github 
> markdown implementation:
> 
>   https://enterprise.github.com/downloads/en/markdown-cheatsheet.pdf
> 
> The following characters may need to be backslash escaped, although it does 
> not cover HTML stuff.
> 
>   \   backslash
>   `   backtick
>   *   asterisk
>   _   underscore
>   {}  curly braces
>   []  square brackets
>   ()  parentheses
>   #   hash mark
>   +   plus sign
>   -   minus sign (hyphen)
>   .   dot
>   !   exclamation
> 
> Another source https://genius.com/3057216 suggests (* # / ( ) [ ] < >),
> which should protect HTML.
> 
> However, the escaping seems to be the backslash character, NOT using html 
> encoding  as done in your version.
> 
> Where did you find the precise escaping rules for markdown? I do not think 
> that it should be invented...
> 
> 
> I have looked at RST, according to this reference:
> 
>   
> http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#grid-tables
> 
> The good news is that you do not need to handle a special | case because you 
> would only produce clean tables.
> 
> I've tested UTF-8 with plane 1 (你好!) and plane 2 (!) and the alignment seems 
> to worked well, incredible!
> 
>>> My main interest on this was in rst. I`m using markdown only in github 
>>> issues and my knowldge about md is poor.
> 
> Then maybe only do RST?!
> 
> It looks much simpler anyway, and if you do MARKDOWN the support needs to be 
> clean.
> 
> About the code:
> 
> I'm still at odds with the code which needs to test for markdown to call for 
> different functions in multiple places. If you keep md and in order to avoid 
> that, I would suggest to extend the pg_wcs* functions with a list of 
> caracters which may have different sizes with additionnal args, say:
> 
>  pg_wcssize(// same args, plus:
> char * escaped_chars, // will require escaping
> int escape_len, // how many chars added when escaping
> int nllen // len of newline if substituted
> );
> 
> So that pg_wcssize(..., "\r", 1, 1) would behave as before (\n and \t are 
> rather special cases), and the various constants could be held in the format 
> description so the whole thing would be parametric.
> 
> Same approach with format.
> 
> That would allow to simplify the code significantly and to share it between 
> MARKDOWN and others. Also, the multiple "else if" list would be simplified by 
> using strchr or the escaped_chars string.

This patch was moved into the current Commitfest marked “Waiting for author”
with the above review.  Have you had a chance to work on it addressing the
review comments such that we can expect a new version within this CF?

cheers ./daniel

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

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[Daniel Gustafsson]

> On 30 Mar 2017, at 09:11, Ideriha, Takeshi  
> wrote:
> 
> Thank you for prompt check!
>  
> >As per above test steps, it doesn't produce the results and doesn't
> >generate the error also. I feel this needs to be fixed.
>  
> >As we are at the end of commitfest, it is better you can move it
> >to next one commitfest and provide an updated patch to solve the
> >above problem.
>  
> I tottaly agreed.
> I moved to next CF with waiting on author.

This patch was moved to the current commitfest (and to the previous one from
the 201701 CF).  Have you had the chance to address the review comments such
that there is an update expected within this CF?

cheers ./daniel

-- 
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] Patches that don't apply or don't compile: 2017-09-12

[Daniel Gustafsson]

> On 12 Sep 2017, at 23:54, Tomas Vondra  wrote:
> 
> On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
>> Hello, hackers!
>> 
>> Thanks to the work of Thomas Munro now we have a CI for the patches on
>> the commitfest [1]. Naturally there is still room for improvement, but
>> in any case it's much, much better than nothing.
>> 
>> After a short discussion [2] we agreed (or at least no one objected)
>> to determine the patches that don't apply / don't compile / don't
>> pass regression tests and have "Needs Review" status, change the
>> status of these patches to "Waiting on Author" and write a report
>> (this one) with a CC to the authors. As all we know, we are short on
>> reviewers and this action will save them a lot of time. Here [3] you
>> can find a script I've been using to find such patches.
>> 
>> I rechecked the list manually and did my best to exclude the patches 
>> that were updated recently or that depend on other patches. However 
>> there still a chance that your patch got to the list by a mistake.
>> In this case please let me know.>
> 
> With all due respect, it's hard not to see this as a disruption of the
> current CF. I agree automating the patch processing is a worthwhile
> goal, but we're not there yet and it seems somewhat premature.
> 
> Let me explain why I think so:
> 
> (1) You just changed the status of 10-15% open patches. I'd expect
> things like this to be consulted with the CF manager, yet I don't see
> any comments from Daniel. Considering he's been at the Oslo PUG meetup
> today, I doubt he was watching hackers very closely.

Correct, I’ve been travelling and running a meetup today so had missed this on
-hackers.

> (2) You gave everyone about 4 hours to object, ending 3PM UTC, which
> excludes about one whole hemisphere where it's either too early or too
> late for people to respond. I'd say waiting for >24 hours would be more
> appropriate.

Agreed.

> I object to changing the patch status merely based on the script output.
> It's a nice goal, but we need to do the legwork first, otherwise it'll
> be just annoying and disrupting.

I too fear that automating the state change will move patches away from “Needs
review” in too many cases unless there is manual inspection step.  Colliding on
Oids in pg_proc comes to mind as a case where the patch won’t build, but the
reviewer can trivially fix that locally and keep reviewing.

> I suggest we inspect the reported patches manually, investigate whether
> the failures are legitimate or not, and eliminate as many false
> positives as possible. Once we are happy with the accuracy, we can
> enable it again.

This seems to summarize the sentiment in the other thread, this is absolutely a
step in the right direction, we just need to tweak it with human knowledge
before it can be made fully automatic to avoid false positives.  The last thing
we want is for the community to consider CF status changes/updates to be crying
wolf, there are few enough reviewers as there is.

cheers ./daniel

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

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Daniel Gustafsson]

> On 08 Sep 2017, at 19:14, Simon Riggs  wrote:
> 
> On 6 September 2017 at 07:43, Robert Haas  wrote:
> 
>> LET custom_plan_tries = 0 IN SELECT ...
> 
> Tom has pointed me at this proposal, since on another thread I asked
> for something very similar. (No need to reprise that discussion, but I
> wanted prepared queries to be able to do SET work_mem = X; SELECT).
> This idea looks a good way forward to me.
> 
> Since we're all in roughly the same place, I'd like to propose that we
> proceed with the following syntax... whether or not this precisely
> solves OP's issue on this thread.
> 
> 1. Allow SET to set multiple parameters...
> SET guc1 = x, guc2 = y
> This looks fairly straightforward
> 
> 2. Allow a SET to apply only for a single statement
> SET guc1 = x, guc2 = y FOR stmt
> e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
> Internally a GUC setting already exists for a single use, via
> GUC_ACTION_SAVE, so we just need to invoke it.

This syntax proposal makes sense, +1.  My immediate thought was that the
per-statement GUCs were sort of like options, and most options in our syntax
are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..;

cheers ./daniel


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

[HACKERS] Commitfest 201709 Progress

[Daniel Gustafsson]

We are now a few days in on Commitfest 201709 and there has been lots of
activity on the patches.  22% of the patches have been committed* with another
10% being marked Ready for Committer.  Thank you all for the hard work!

There is still a lot to do though, this fest has a recordbreaking 256 patches
and will need lots more hard work to close more patches.  There are, at the
time of writing, 131 patches that need review with 65 of those also waiting for
a reviewer.  Quite a few of these patches are large and attention early in the
fest will be crucial to iron out the details.

If you’ve never reviewed a patch before, there won’t be a better time to get
involved.  Remember that everyones input is just as valuable, PostgreSQL is a
community effort!

Again, thank you for all the hard work so far.

cheers ./daniel, as CFM 201709

* The keen observer will have noted that some patches were committed even
  before the commitfest began, but hey, that still counts.

-- 
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] Making clausesel.c Smarter

[Daniel Gustafsson]

> On 06 Sep 2017, at 07:13, David Rowley <david.row...@2ndquadrant.com> wrote:
> 
> On 6 September 2017 at 00:43, Daniel Gustafsson <dan...@yesql.se> wrote:
>> This patch was moved to the currently open Commitfest.  Given the above
>> comment, is the last patch in this thread still up for review, or are you
>> working on a new version?  Just to avoid anyone reviewing an outdated 
>> version.
> 
> Hi Daniel,
> 
> I've not had time to work on a new version yet and I can't imagine
> that I will for quite some time.
> 
> The idea I had to remove the quadratic search on the list was to add
> to or modify equal() in equalfuncs.c to have it return -1, 0, +1 and
> use that as a comparison function in a binary search tree. The Btree
> would complement List as a way of storing Nodes in no particular
> order, but in a way that a Node can be found quickly. There's likely
> more than a handful of places in the planner that would benefit from
> this already.
> 
> It's not a 5-minute patch and it probably would see some resistance,
> so won't have time to look at this soon.
> 
> If the possibility of this increasing planning time in corner cases is
> going to be a problem, then it might be best to return this with
> feedback for now and I'll resubmit if I get time later in the cycle.

For now I’ve marked this “Waiting on Author” awaiting a new patch for the
community to consider.  If there is no time to hack on this in the current CF
(which seems likely given your mail above) I’ll move it to the next CF as this
one draws to a close.

cheers ./daniel



-- 
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] Copyright in partition.h and partition.c

[Daniel Gustafsson]

> On 06 Sep 2017, at 02:56, Amit Langote  wrote:
> 
> On 2017/09/05 21:14, Tom Lane wrote:
>> Amit Langote  writes:
>>> On 2017/09/05 15:48, Etsuro Fujita wrote:
 Here is the copyright in partition.h:
 
  * Copyright (c) 2007-2017, PostgreSQL Global Development Group
 
 I think it's reasonable that that matches the copyright in partition.c,
 but partition.c has:
 
  * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
 
 Is that intentional?
>> 
>>> No, it's unintentional.  The difference may have resulted from copying
>>> different files to become partition.h and partition.c, respectively.
>> 
>>> Maybe, we should change both to say 2016-2017?
>> 
>>> I don't know the exact rule for how we determine those years.  Is there
>>> some rule in place about that?  When I look at execParallel.c, which
>>> supposedly got introduced into the tree recently, I see 1996-2017.  OTOH,
>>> the files in contrib/bloom all have 2016-2017.
>> 
>> Our usual practice is to write the copyright like it is in partition.c
>> even in new files.  This avoids any question about whether any of the
>> code was copied-and-pasted from somewhere else in PG.  Even if not one
>> word in the file can be traced to code that was somewhere else before,
>> it seems to me that this is an appropriate thing to do, to give due
>> credit to those who came before us.
> 
> Agreed.
> 
>> In short: we should make partition.h's copyright look like partition.c's
>> not vice versa.
> 
> Attached patch does that.

This reminded me that I’d seen one of these before while hacking, and with some
grep and xargs abuse I spotted one more (there might be more that my command
line fu didn’t catch though).  Attached could perhaps be included with the
above patch?

Perhaps the copyright script should be expanded to catch these?  (and I
volunteer to attempt that unless it’s deemed an uninteresting feature)

cheers ./daniel



header_copyright.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] A note about debugging TAP failures

[Daniel Gustafsson]

> On 05 Sep 2017, at 18:37, Peter Eisentraut <peter.eisentr...@2ndquadrant.com> 
> wrote:
> 
> On 4/25/17 11:00, Daniel Gustafsson wrote:
>> Makes sense, assuming a “clean slate” to run on seems a reasonable assumption
>> for the test and it makes for simpler code in PostgresNode.  Updated the 
>> patch
>> which now have static datadir names; retains on PG_TESTS_NOCLEAN or any kind 
>> of
>> test failure; and allows for cleaning datadirs without having to use make 
>> clean
>> (seems the wrong size hammer for that nail).
> 
> Committed.  I had to make some changes in the pg_rewind tests.  Those
> make data directories with conflicting names, which did not work anymore
> because the names are no longer random.

Makes sense, +1 on those changes.

> Other than that this is pretty nice.  Thanks!

Thanks for the commit!

cheers ./daniel

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

Re: [HACKERS] Proposal for changes to recovery.conf API

[Daniel Gustafsson]

> On 27 Mar 2017, at 10:27, Simon Riggs  wrote:
> 
> On 7 March 2017 at 23:31, Josh Berkus  wrote:
>> On 03/02/2017 07:13 AM, David Steele wrote:
>>> Hi Simon,
>>> 
>>> On 2/25/17 2:43 PM, Simon Riggs wrote:
 On 25 February 2017 at 13:58, Michael Paquier  
 wrote:
 
> - trigger_file is removed.
> FWIW, my only complain is about the removal of trigger_file, this is
> useful to detect a trigger file on a different partition that PGDATA!
> Keeping it costs also nothing..
 
 Sorry, that is just an error of implementation, not intention. I had
 it on my list to keep, at your request.
 
 New version coming up.
>>> 
>>> Do you have an idea when the new version will be available?
>> 
>> Please?  Having yet another PostgreSQL release go by without fixing
>> recovery.conf would make me very sad.
> 
> I share your pain, but there are various things about this patch that
> make me uncomfortable. I believe we are looking for an improved design
> not just a different design.
> 
> I think the best time to commit such a patch is at the beginning of a
> new cycle, so people have a chance to pick out pieces they don't like
> and incrementally propose changes.
> 
> So I am going to mark this MovedToNextCF, barring objections from
> committers willing to make it happen in this release.

Next CF has now become This CF, has there been any work on this highly sought
after patch?  Would be good to get closure on this early in the v11 cycle.

cheers ./daniel



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

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Daniel Gustafsson]

> On 08 Apr 2017, at 09:42, Pavel Stehule  wrote:
> 
> 2017-04-08 2:30 GMT+02:00 Peter Eisentraut  >:
> On 4/6/17 14:32, Pavel Stehule wrote:
> > I like to see any proposals about syntax or implementation.
> >
> > Using PRAGMA is one variant - introduced by PLpgSQL origin - Ada
> > language. The PRAGMA syntax can be used for PRAGMA autonomous with well
> > known syntax. It scales well  - it supports function, block or command
> > level.
> 
> I had pragmas implemented in the original autonomous transactions patch
> (https://www.postgresql.org/message-id/659a2fce-b6ee-06de-05c0-c8ed6a019...@2ndquadrant.com
>  
> ).
>  However, the difference there is that the behavior is lexical, specific
> to plpgsql, whereas here you are really just selecting run time
> behavior.  So a GUC, and also something that could apply to other
> places, should be considered.
> 
> I'll look there - we coordinate work on that.

This patch was moved to the now started commitfest, and is marked as “Needs
review”.  Based on this thread I will however change it to "waiting for author”,
since there seems to be some open questions.  Has there been any new work done
on this towards a new design/patch?

cheers ./daniel

-- 
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] Making clausesel.c Smarter

[Daniel Gustafsson]

> On 09 Apr 2017, at 12:14, David Rowley  wrote:
> 
> On 8 April 2017 at 09:33, Claudio Freire  wrote:
>> Otherwise, the patch LGTM, but I'd like to solve the quadratic
>> behavior too... are you going to try? Otherwise I could take a stab at
>> it myself. It doesn't seem very difficult.
> 
> I have some ideas in my head in a fairly generic way of solving this
> which I'd like to take care of in PG11.

This patch was moved to the currently open Commitfest.  Given the above
comment, is the last patch in this thread still up for review, or are you
working on a new version?  Just to avoid anyone reviewing an outdated version.

cheers ./daniel

-- 
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] Should we cacheline align PGXACT?

[Daniel Gustafsson]

> On 04 Apr 2017, at 14:58, David Steele  wrote:
> 
> On 4/4/17 8:55 AM, Alexander Korotkov wrote:
>> On Mon, Apr 3, 2017 at 9:58 PM, Andres Freund > 
>>I'm inclined to push this to the next CF, it seems we need a lot more
>>benchmarking here.
>> 
>> No objections.
> 
> This submission has been moved to CF 2017-07.

This CF has now started (well, 201709 but that’s what was meant in above), can
we reach closure on this patch in this CF?

cheers ./daniel

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

Re: [HACKERS] [PROPOSAL] Use SnapshotAny in get_actual_variable_range

[Daniel Gustafsson]

> On 18 Aug 2017, at 08:50, Andrey Borodin  wrote:
> 
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, failed
> Implements feature:   tested, failed
> Spec compliant:   tested, failed
> Documentation:tested, failed
> 
> Hi! I've looked into the patch.
> 
> There is not so much of the code. The code itself is pretty clear and well 
> documented. I've found just one small typo "transactionn" in 
> HeapTupleSatisfiesNonVacuumable() comments.
> 
> Chosen Snapshot type is not a solution to the author's problem at hand. 
> Though implemented SnapshotNonVacuumable is closer to rational choice  than 
> currently used SnapshotDirty for range sketching. As far as I can see this is 
> what is get_actual_variable_range() is used for, despite word "actual" in 
> it's name. 
> The only place where get_actual_variable_range() is used for actual range is 
> preprocessed-out in get_variable_range():
>   /*
>* XXX It's very tempting to try to use the actual column min and max, 
> if
>* we can get them relatively-cheaply with an index probe.  However, 
> since
>* this function is called many times during join planning, that could
>* have unpleasant effects on planning speed.  Need more investigation
>* before enabling this.
>*/
> #ifdef NOT_USED
>   if (get_actual_variable_range(root, vardata, sortop, min, max))
>   return true;
> #endif
> 
> I think it would be good if we could have something like "give me actual 
> values, but only if it is on first index page", like conditional locks. But I 
> think this patch is a step to right direction.

Thanks for your review!  Based on your review and conclusions, I’ve bumped the
status to “Ready for Committer”. If you believe another status would be more
appropriate, please go in an update.

cheers ./daniel



-- 
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] WAL logging problem in 9.4.3?

[Daniel Gustafsson]

> On 13 Apr 2017, at 11:42, Kyotaro HORIGUCHI  
> wrote:
> 
> At Thu, 13 Apr 2017 13:52:40 +0900, Michael Paquier 
>  wrote in 
> 
>> On Tue, Apr 11, 2017 at 5:38 PM, Kyotaro HORIGUCHI
>>  wrote:
>>> Sorry, what I have just sent was broken.
>> 
>> You can use PROVE_TESTS when running make check to select a subset of
>> tests you want to run. I use that all the time when working on patches
>> dedicated to certain code paths.
> 
> Thank you for the information. Removing unwanted test scripts
> from t/ directories was annoyance. This makes me happy.
> 
 - Relation has new members no_pending_sync and pending_sync that
  works as instant cache of an entry in pendingSync hash.
 - Commit-time synchronizing is restored as Michael's patch.
 - If relfilenode is replaced, pending_sync for the old node is
  removed. Anyway this is ignored on abort and meaningless on
  commit.
 - TAP test is renamed to 012 since some new files have been added.
 
 Accessing pending sync hash occurred on every calling of
 HeapNeedsWAL() (per insertion/update/freeze of a tuple) if any of
 accessing relations has pending sync.  Almost of them are
 eliminated as the result.
>> 
>> Did you actually test this patch? One of the logs added makes the
>> tests a long time to run:
> 
> Maybe this patch requires make clean since it extends the
> structure RelationData. (Perhaps I saw the same trouble.)
> 
>> 2017-04-13 12:11:27.065 JST [85441] t/102_vacuumdb_stages.pl
>> STATEMENT:  ANALYZE;
>> 2017-04-13 12:12:25.766 JST [85492] LOG:  BufferNeedsWAL: pendingSyncs
>> = 0x0, no_pending_sync = 0
>> 
>> -   lsn = XLogInsert(RM_SMGR_ID,
>> -XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
>> +   rel->no_pending_sync= false;
>> +   rel->pending_sync = pending;
>> +   }
>> 
>> It seems to me that those flags and the pending_sync data should be
>> kept in the context of backend process and not be part of the Relation
>> data...
> 
> I understand that the context of "backend process" means
> storage.c local. I don't mind the context on which the data is,
> but I found only there that can get rid of frequent hash
> searching. For pending deletions, just appending to a list is
> enough and costs almost nothing, on the other hand pendig syncs
> are required to be referenced, sometimes very frequently.
> 
>> +void
>> +RecordPendingSync(Relation rel)
>> I don't think that I agree that this should be part of relcache.c. The
>> syncs are tracked should be tracked out of the relation context.
> 
> Yeah.. It's in storage.c in the latest patch. (Sorry for the
> duplicate name). I think it is a kind of bond between smgr and
> relation.
> 
>> Seeing how invasive this change is, I would also advocate for this
>> patch as only being a HEAD-only change, not many people are
>> complaining about this optimization of TRUNCATE missing when wal_level
>> = minimal, and this needs a very careful review.
> 
> Agreed.
> 
>> Should I code something? Or Horiguchi-san, would you take care of it?
>> The previous crash I saw has been taken care of, but it's been really
>> some time since I looked at this patch...
> 
> My point is hash-search on every tuple insertion should be evaded
> even if it happens rearely. Once it was a bit apart from your
> original patch, but in the latest patch the significant part
> (pending-sync hash) is revived from the original one.

This patch has followed along since CF 2016-03, do we think we can reach a
conclusion in this CF?  It was marked as "Waiting on Author”, based on
developments since in this thread, I’ve changed it back to “Needs Review”
again.

cheers ./daniel




-- 
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] Refactoring identifier checks to consistently use strcmp

[Daniel Gustafsson]

> On 17 Aug 2017, at 11:08, Daniel Gustafsson <dan...@yesql.se> wrote:
> 
>> On 16 Aug 2017, at 17:51, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 
>> Heikki Linnakangas <hlinn...@iki.fi> writes:
>>> This no longer works:
>> 
>>> postgres=# CREATE TEXT SEARCH DICTIONARY public.simple_dict (
>>>TEMPLATE = pg_catalog.simple,
>>>"STOPWORds" = english
>>> );
>>> ERROR:  unrecognized simple dictionary parameter: "STOPWORds"
>> 
>>> In hindsight, perhaps we should always have been more strict about that 
>>> to begin wtih, but let's not break backwards-compatibility without a 
>>> better reason. I didn't thoroughly check all of the cases here, to see 
>>> if there are more like this.
>> 
>> You have a point, but I'm not sure that this is such a bad compatibility
>> break as to be a reason not to change things to be more consistent.
> 
> I agree with this, but I admittedly have no idea how common the above case
> would be in the wild.
> 
>>> It'd be nice to have some kind of a rule on when pg_strcasecmp should be 
>>> used and when strcmp() is enough. Currently, by looking at the code, I 
>>> can't tell.
>> 
>> My thought is that if we are looking at words that have been through the
>> parser, then it should *always* be plain strcmp; we should expect that
>> the parser already did the appropriate case-folding.
> 
> +1
> 
>> pg_strcasecmp would be appropriate, perhaps, if we're dealing with stuff
>> that somehow came in without going through the parser.
> 
> In that case it would be up to the consumer of the data to handle required
> case-folding for the expected input, so pg_strcasecmp or strcmp depending on
> situation.

This patch has been marked “Waiting on Author”, but I’m not sure what the
concensus of this thread came to with regards to quoted keywords and backwards
compatibility.  There seems to be a 2-1 vote for allowing a break, and forcing
all keywords out of the parser to be casefolded. Any other opinions?

cheers ./daniel



-- 
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 the commit to a patch's thread

[Daniel Gustafsson]

> On 03 Sep 2017, at 21:12, Magnus Hagander <mag...@hagander.net> wrote:
> 
> On Sun, Sep 3, 2017 at 8:55 PM, Daniel Gustafsson <dan...@yesql.se 
> <mailto:dan...@yesql.se>> wrote:
> 
> That leaves us back at parsing/scraping -committers and adding the mailthread.
> For this CF I can add the commits manually, since attaching a thread isn’t
> limited to threads in -hackers.  This way we have more time to figure out an
> automated way for the next CF.  I’ve added the commit for my patch here as an
> example:
> 
> https://commitfest.postgresql.org/14/1245/ 
> <https://commitfest.postgresql.org/14/1245/>
> 
> That's a good start.

I will do that for commited patches.

> We can also add a separate field that's just a comma separated list of commit 
> hashes that auto-links to the git server if we want. *That* would be a 
> trivial addition. Actually auto-populating it would probably be a lot less 
> so, but for a manually managed one it should be fairly easy.

We can do that as well in case we want to, one doesn’t exclude the other.
Having both a link to the thread on -committers as well as a link to Git would
be neat (the former will neatly capture discussions on -committers that lead to
follow-up patches, something the latter will struggle with since it will be
lots extra work).

cheers ./daniel

-- 
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] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 02 Sep 2017, at 02:21, Thomas Munro <thomas.mu...@enterprisedb.com> wrote:
> 
> On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
>> Good point.  I’ve attached a new version which issues a NOTICE on truncation
>> and also addresses all other comments so far in this thread.  Thanks a lot 
>> for
>> the early patch reviews!
> 
> FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:
> 
> make[3]: Entering directory
> `/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
> cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> 772
> 972
> make[3]: *** [postgres.bki] Error 1

Thanks, I hadn’t spotted that yet.  Attached is an updated patch using new OIDs
to make it compile again.

cheers ./daniel



terminate_msg_v4.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] adding the commit to a patch's thread

[Daniel Gustafsson]

> On 03 Sep 2017, at 19:33, Magnus Hagander <mag...@hagander.net> wrote:
> 
> On Sun, Sep 3, 2017 at 12:00 AM, Daniel Gustafsson <dan...@yesql.se 
> <mailto:dan...@yesql.se>> wrote:
> > On 01 Sep 2017, at 15:40, Robert Haas <robertmh...@gmail.com 
> > <mailto:robertmh...@gmail.com>> wrote:
> >
> > On Fri, Sep 1, 2017 at 4:26 AM, Alvaro Herrera <alvhe...@alvh.no-ip.org 
> > <mailto:alvhe...@alvh.no-ip.org>> wrote:
> >> Erik Rijkers wrote:
> >>> Would it be possible to change the commitfest a bit and make it possible 
> >>> to
> >>> add the commit (or commit-message, or hash) to the thread in the
> >>> commitfest-app.
> >>
> >> +1 to add one or more commit hashes to CF entry metadata.
> >>
> >> (Back-filling for old entries welcome)
> >
> > Couldn't the CF app scrape the commit messages for references to
> > threads, and if the commit message points to a thread with exactly 1
> > patch record, associate the commit to that patch?
> 
> Since there is a Gitlink field in the patch metadata, we could start simple
> with setting that to link to the commit on pg gitweb?  While adding the thread
> of the commit from -committers would be great as well, this gets us off the
> ground quickly.
> 
> The original idea behind the gitlink field was to link to a git repo/branch 
> representing the patch, for people who preferred to publish full branches for 
> those that want it.
> 
> This has been done for a grand total of 43 patches throughout (out of a total 
> of 1231).
> 
> Not sure if that's enough to say "let's not repurpose it”?

My thinking was that it wasn’t really repurposing, since the commit is quite
representative of the patch and comma separated list could house both
(especially since the former usecase is quite rate).  Looking at the code
however, the Gitlink is a UrlField which I believe won’t support that so it’s
dead in the water either way.

That leaves us back at parsing/scraping -committers and adding the mailthread.
For this CF I can add the commits manually, since attaching a thread isn’t
limited to threads in -hackers.  This way we have more time to figure out an
automated way for the next CF.  I’ve added the commit for my patch here as an
example:

https://commitfest.postgresql.org/14/1245/

cheers ./daniel

-- 
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 the commit to a patch's thread

[Daniel Gustafsson]

> On 01 Sep 2017, at 15:40, Robert Haas  wrote:
> 
> On Fri, Sep 1, 2017 at 4:26 AM, Alvaro Herrera  
> wrote:
>> Erik Rijkers wrote:
>>> Would it be possible to change the commitfest a bit and make it possible to
>>> add the commit (or commit-message, or hash) to the thread in the
>>> commitfest-app.
>> 
>> +1 to add one or more commit hashes to CF entry metadata.
>> 
>> (Back-filling for old entries welcome)
> 
> Couldn't the CF app scrape the commit messages for references to
> threads, and if the commit message points to a thread with exactly 1
> patch record, associate the commit to that patch?

Since there is a Gitlink field in the patch metadata, we could start simple
with setting that to link to the commit on pg gitweb?  While adding the thread
of the commit from -committers would be great as well, this gets us off the
ground quickly.

Unless there are objections, I’ll ensure that this is maintained during this CF
as part of my commitfest tinkering.

cheers ./daniel

-- 
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] GnuTLS support

[Daniel Gustafsson]

> On 01 Sep 2017, at 20:00, Robert Haas  wrote:
> 
> On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
 I have seen discussions from time to time about OpenSSL and its licensing
 issues so I decided to see how much work it would be to add support for
 another TLS library, and  I went with GnuTLS since it is the library I know
 best after OpenSSL and it is also a reasonably popular library.
>> 
>>> Thanks for working on this.  I think it's good for PostgreSQL to have
>>> more options in this area.
>> 
>> +1.  We also have a patch in the queue to support macOS' TLS library,
>> and I suppose that's going to be facing similar issues.  It would be
>> a good plan, probably, to try to push both of these to conclusion in
>> the same development cycle.
> 
> The thing which I think would save the most aggravation - at least for
> my employer - is a Windows SSL implementation.

In 53ea546e.6020...@vmware.com, an early version of SChannel support was posted
by Heikki.  If anyone is keen to pick up the effort that would most likely be a
good starting point.

> Relying on OpenSSL
> means that every time OpenSSL puts out a critical security fix, we've
> got to rewrap all the Windows installers to pick up the new version.
> If we were relying on what's built into Windows, it would be
> Microsoft's problem.  Granted, it's not anybody's job to solve
> EnterpriseDB's problems except EnterpriseDB, but users might like it
> too -- and anyone else who is building Windows installers for
> PostgreSQL.
> 
> Depending on macOS TLS instead of OpenSSL has similar advantages, of
> course, just for a somewhat less common platform.

I think providing alternatives to OpenSSL on platforms where OpenSSL can’t be
relied on to be already available (Windows and macOS come to mind) would be a
great thing for many users and app developers.

cheers ./daniel

-- 
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] GnuTLS support

[Daniel Gustafsson]

> On 01 Sep 2017, at 19:10, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
> 
>>> There are currently two failing SSL tests which at least to me seems more
>>> like they test specific OpenSSL behaviors rather than something which need
>>> to be true for all SSL libraries.
> 
>> I don't know what we should do about these issues.
> 
> Maybe the SSL test suite needs to be implementation-specific as well.

To properly test the macOS Secure Transport support we will need to use
Keychain files on top of plain PEM files, so I think we have to.  That being
said, we should probably define a (as large possible) minimum set which applies
to all to ensure compatability between different frontends and backends.

cheers ./daniel

-- 
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] Upcoming commit fest will begin soon

[Daniel Gustafsson]

> On 01 Sep 2017, at 05:33, Michael Paquier  wrote:
> 
> Another question is: who would like to become the CF manager for this
> time? This commit fest is very large, so it is going to be difficult
> for one person to do the whole thing, hence at least 2 people would be
> welcome. Please note that I am not directly volunteering for this one,
> but per my number of patches I need to look at many things, so I may
> be considered as a sort of co-manager ;)
> New heads for this exercise would be nice though.

I volunteer to be co-managing this CF.  Since I’ve never done it before, and
the CF in question is rather large I wouldn’t mind it co-managing it with
someone as you mention.

cheers ./daniel

-- 
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] Support for Secure Transport SSL library on macOS as OpenSSL alternative

[Daniel Gustafsson]

> On 19 Aug 2017, at 23:13, Thomas Munro <thomas.mu...@enterprisedb.com> wrote:
> 
> On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro
> <thomas.mu...@enterprisedb.com <mailto:thomas.mu...@enterprisedb.com>> wrote:
>> On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
>>> Attached is an updated set of patches, rebased on top of master, with bug 
>>> fixes
>>> and additional features missing in the first set.  While not complete 
>>> (yet), in
>>> case anyone is testing this I’d rather send a fresh batch rather than 
>>> sitting
>>> on them too long while I keep hacking at the docs.  While not every part of
>>> this rather large changeset has been touched, this includes all the patches 
>>> for
>>> completeness sake.
>> 
>> Hi,
>> 
>> +#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT)
>> #define USE_SSL
>> +#if defined(USE_OPENSSL)
>> +#define SSL_LIBRARY "OpenSSL"
>> +#elif defined(USE_SECURETRANSPORT)
>> +#define SSL_LIBRARY "Secure Transport"
>> +#endif
>> #endif
>> 
>> If you configure with neither --with-securetransport nor
>> --with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c
>> doesn't compile:
>> 
>> ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith
>> -Wdeclaration-after-statement -Wendif-labels
>> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
>> -fwrapv -fexcess-precision=standard -g -O2 -I. -I.
>> -I../../../../src/include  -D_GNU_SOURCE   -c -o guc.o guc.c
>> guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function)
>>   SSL_LIBRARY,
>>   ^~~
>> 
>> I guess it should have a fallback definition, though I don't know what
>> it should be.
> 
> Or maybe the guc should only exist if SSL_LIBRARY is defined?

I think the intended use case of the GUC should drive the decision on fallback.
If the GUC isn’t supposed to be a way to figure out if the server was built
with SSL support, then not existing in non-SSL backends is fine.  If, however,
we want to allow using the GUC to see if the server has SSL support, then there
needs to be a “None” or similar value for that case.

Personally I think there is risk of regrets down the line if this GUC is used
for two things, but thats more of a gut feeling than scientifically studied.

Clearly there shouldn’t be a compilation error in either case, sorry about
missing that in the submission.

cheers ./daniel

-- 
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] Refactoring identifier checks to consistently use strcmp

[Daniel Gustafsson]

> On 16 Aug 2017, at 17:51, Tom Lane  wrote:
> 
> Heikki Linnakangas  writes:
>> This no longer works:
> 
>> postgres=# CREATE TEXT SEARCH DICTIONARY public.simple_dict (
>> TEMPLATE = pg_catalog.simple,
>> "STOPWORds" = english
>> );
>> ERROR:  unrecognized simple dictionary parameter: "STOPWORds"
> 
>> In hindsight, perhaps we should always have been more strict about that 
>> to begin wtih, but let's not break backwards-compatibility without a 
>> better reason. I didn't thoroughly check all of the cases here, to see 
>> if there are more like this.
> 
> You have a point, but I'm not sure that this is such a bad compatibility
> break as to be a reason not to change things to be more consistent.

I agree with this, but I admittedly have no idea how common the above case
would be in the wild.

>> It'd be nice to have some kind of a rule on when pg_strcasecmp should be 
>> used and when strcmp() is enough. Currently, by looking at the code, I 
>> can't tell.
> 
> My thought is that if we are looking at words that have been through the
> parser, then it should *always* be plain strcmp; we should expect that
> the parser already did the appropriate case-folding.

+1

> pg_strcasecmp would be appropriate, perhaps, if we're dealing with stuff
> that somehow came in without going through the parser.

In that case it would be up to the consumer of the data to handle required
case-folding for the expected input, so pg_strcasecmp or strcmp depending on
situation.

cheers ./daniel


-- 
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] Support for Secure Transport SSL library on macOS as OpenSSL alternative

[Daniel Gustafsson]

> On 03 Aug 2017, at 19:27, Michael Paquier <michael.paqu...@gmail.com> wrote:
> 
> On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson <dan...@yesql.se> wrote:
>> In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I
>> presented a WIP patch for adding support for the Apple Secure Transport SSL
>> library on macOS as, an alternative to OpenSSL.  That patch got put on the
>> backburner for a bit, but I’ve now found the time to make enough progress to
>> warrant a new submission for discussions on this (and hopefully help 
>> hacking).
>> 
>> It is a drop-in replacement for the OpenSSL code, and supports all the same
>> features and options, except for two things: compression is not supported and
>> the CRL cannot be loaded from a plain PEM file.  A Keychain must be used for
>> that instead.
> 
> Is there a set of APIs to be able to get server certificate for the
> frontend and the backend, and generate a hash of it? That matters for
> channel binding support of SCRAM for tls-server-end-point.

I believe we can use SSLCopyPeerTrust() for that.  Admittedly I haven’t looked
at that yet so need to get my head around channel binding, but it seems to fit
the bill.

> There were no APIs to get the TLS finish message last time I looked at OSX
> stuff, which mattered for tls-unique.  It would be nice if we could get one.


Yeah, AFAICT there is no API for that.

cheers ./daniel

-- 
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] Support for Secure Transport SSL library on macOS as OpenSSL alternative

[Daniel Gustafsson]

> On 03 Aug 2017, at 13:06, Heikki Linnakangas <hlinn...@iki.fi> wrote:
> 
> On 08/03/2017 01:02 PM, Daniel Gustafsson wrote:
>> 
>> The frontend has support for using PEM files for certificates and keys.  It 
>> can
>> also look up the key for the certificate in a Keychain, or both certificate 
>> and
>> key in a Keychain.  The root certificate is still just read from a PEM file.
> 
> Why can't you have the root certificate in the keychain, too? Just not 
> implemented yet, or is there some fundamental problem with it?

Just not implemented yet, awaiting Keychain discussions.

>> The existence of an sslcert config trumps a keychain, but when a keychain is
>> used I’m currently using the sslcert parameter (awaiting a discussion on how 
>> to
>> properly do this) for the certificate label required to search the keychain.
>> There is a new frontend parameter called “keychain” with which a path to a
>> custom Keychain file can be passed.  If set, this Keychain will be searched 
>> as
>> well as the default.  If not, only the default user Keychain is used.  There 
>> is
>> nothing that modifies the Keychain in this patch, it can read identities
>> (certificates and its key) but not alter them in any way.
> 
> OpenSSL also has a mechanism somewhat similar to the keychain, called 
> "engines". You can e.g. keep the private key corresponding a certificate on a 
> smart card, and speak to it with an OpenSSL "smart card reader" engine. If 
> you do that, the 'sslkey' syntax is ":". Perhaps we 
> should adopt that syntax here as well? For example, to read the client 
> certificate from the key chain, you would use libpq options like 
> "keychain=/home/heikki/my_keychain sslcert=keychain:my_client_cert”.

Thats definitely an option, although it carries a bit redudancy in this case
which can confuse users.  With “keychain=/foo/bar.keychain sslcert=my_cert”,
did the user mean a file called my_cert or is it a reference to a cert in the
keychain?  Nothing that strict parsing rules, good errormessages and
documentation can’t solve but needs careful thinking.

>> “keychain” is obviously a very Secure Transport specific name, and I 
>> personally
>> think we should avoid that.  Any new configuration added here should take
>> future potential implementation into consideration such that avoid the need 
>> for
>> lots of backend specific knobs.  “sslcertstore” comes to mind as an
>> alternative, but we’d also need parameters to point into the certstore for
>> finding what we need.  Another option would be to do a URL based scheme
>> perhaps.
> 
> I wouldn't actually mind using implementation-specific terms like "keychain" 
> here. It makes it clear that it's implementation-specific. I think it would 
> be confusing, to use the same generic option name, like "sslcertstore", for 
> both a macOS keychain and e.g. the private key store in Windows. Or GNU 
> Keyring. In the worst case, you might even have multiple such "key stores" on 
> the same system, so you'd anyways need a way to specify which one you mean.

Thats a good point.

> Actually, perhaps it should be made even more explicit, and call it 
> "secure_transport_keychain". That's quite long, though.

Yeah, secure_transport_ is a pretty long prefix.

> Wrt. keychains, is there a system-global or per-user keychain in macOS? And 
> does this patch use them? If you load a CRL into a global keychain, does it 
> take effect?

Each user has a default Keychain , and on top of that you can create as many
Keychains as you want (a Keychain is really just a database file containing
secret things).  The frontend use the default one for lookups unless the
keychain parameter is set in which case it uses both.

When evaluating trust, Secure Transport will use the default Keychain even if
not explicitly opened (as in the backend code).  It does however only search
for intermediate certificates and not root certs/CRLs.  Adding a policy object
for using CRLs should force it to afaik, but we’d need to support additional
keychains (if only to be able to test without polluting the default).

>> Testing
>> ===
>> In order to test this we need to provide an alternative to the openssl calls 
>> in
>> src/test/ssl/Makefile for Secure Transport.
> 
> Those openssl commands are only needed to re-generate the test certificates. 
> The certificates are included in the git repository, so you only need to 
> re-generate them if you want to modify them or add new ones. I think it's OK 
> to require the openssl tool for that, it won't be needed just to run the test 
> suite.

Ah, of course.  We still need support for re-generating Keycha

[HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative

[Daniel Gustafsson]

In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I
presented a WIP patch for adding support for the Apple Secure Transport SSL
library on macOS as, an alternative to OpenSSL.  That patch got put on the
backburner for a bit, but I’ve now found the time to make enough progress to
warrant a new submission for discussions on this (and hopefully help hacking).

It is a drop-in replacement for the OpenSSL code, and supports all the same
features and options, except for two things: compression is not supported and
the CRL cannot be loaded from a plain PEM file.  A Keychain must be used for
that instead.


Current state
=
The frontend and backend are implemented more or less fully, the two main
things missing being the CRL support (further details below) and loading DH
files (to support the GUC added in c0a15e07cd).  All non-CRL tests but one are
passing.  The failing test is in the frontend and it also fails when running
against an OpenSSL backend, but I can’t find where the logic is flawed and
could do with some help there.


Threads
===
Just like the CFLocaleCopyCurrent() call referenced in postmaster.c, the Secure
Transport APIs makes the process multithreaded.  To keep this out of the
postmaster, and contained in the backend, I’ve moved all functionality to
open_server and left init empty.  I could definitely need some clues on how to
properly handle this, or if it’s a complete showstopper.


Keychains
=
The frontend has support for using PEM files for certificates and keys.  It can
also look up the key for the certificate in a Keychain, or both certificate and
key in a Keychain.  The root certificate is still just read from a PEM file.
The existence of an sslcert config trumps a keychain, but when a keychain is
used I’m currently using the sslcert parameter (awaiting a discussion on how to
properly do this) for the certificate label required to search the keychain.

There is a new frontend parameter called “keychain” with which a path to a
custom Keychain file can be passed.  If set, this Keychain will be searched as
well as the default.  If not, only the default user Keychain is used.  There is
nothing that modifies the Keychain in this patch, it can read identities
(certificates and its key) but not alter them in any way.

The backend is only supporting PEM files at this point.

Once we have support for Keychains, we can however use them for additionally
supporting other Secure Transport features like OCSP etc.

“keychain” is obviously a very Secure Transport specific name, and I personally
think we should avoid that.  Any new configuration added here should take
future potential implementation into consideration such that avoid the need for
lots of backend specific knobs.  “sslcertstore” comes to mind as an
alternative, but we’d also need parameters to point into the certstore for
finding what we need.  Another option would be to do a URL based scheme
perhaps.


Certificate Revocation
==
Secure Transport supports loading CRL lists into Keychain files, the command
line certtool can for example do that.  When doing the trust evaluation on the
connection, a policy can be added which enables revocation checking via CRL.  I
have however been unable to figure out how to load a CRL list programmatically,
and as far as I can tell there is no API for this.  The certtool utility is
using the low-level CSSM APIs for this it seems, but adding that level of
complexity doesn’t seem worth it for us to maintain (I did a test and it turned
big, ugly and messy).

Unless someone knows how to implement this, we’d be limited to requiring the
use of a Keychain file for CRL support.  This would break drop-in replacement
support, but supporting certificate revocation seems more important.


Platform Support

I’ve tested this on 10.11 El Capitan and 10.12 Sierra, which are the systems I
have access to.  Supporting prairiedog and dromedary in the buildfarm will
probably be too hard (if at all possible), but down to 10.9 Mavericks should be
quite possible (if someone has the required systems to test on).  It adds a
dependency on Core Foundation on top of Secure Transport, no other macOS APIs
are used.


Testing
===
In order to test this we need to provide an alternative to the openssl calls in
src/test/ssl/Makefile for Secure Transport.  On top of that, code to generate
Keychains is required.  The certtool application can do all the Keychain
generations (I think) but this is still left open.  The main thing to figure
out here is how to avoid having to type in the Keychain password in a modal GUI
that Secure Transport pops up.  Since a Keychain can be passwordless it should
be doable, but the right certtool incantations for that is still evading me.
I’ve included a show-and-tell patch for this which I’ve used locally for
testing during hacking.


Documentation
=
I have started fiddling with this a little, but to avoid spending time on 

Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files

[Daniel Gustafsson]

> On 27 Jul 2017, at 19:40, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <dan...@yesql.se> writes:
>>> On 19 Jun 2017, at 17:32, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> So, if we're getting into enforcing consistency in describe.c, there's
>>> lots to do.
> 
>> Addressed in attached patch, see list of patches below.
> 
> I've pushed most of this.

Thanks!

> listTables and listDbRoleSettings print a custom message rather than
> an empty table for no matches (but in QUIET mode they just do the
> latter).  I think that's actually a good decision for listDbRoleSettings,
> because the user might be confused about which pattern is which, and
> we can straighten him out with a custom error message.  But the only
> good argument for listTables behaving that way is that people are used
> to it.  

Which is a pretty good argument for not changing it.

> Should we override backwards compatibility to the extent of
> dropping the custom messages in listTables, and just printing an empty
> table-of-tables?
> 
>> 0004 - Most verbose metacommands include additional information on top of 
>> what
>> is in the normal output, while \dRp+ dropped two columns (moving one to the
>> title).  This include the information from \dRp in \dRp+.  Having the pubname
>> in the title as well as the table is perhaps superfuous, but consistent with
>> other commands so opted for it.
> 
> I'm not sure about this one.  It definitely seems bogus that \dRp+ is
> omitting the owner column, but I'm less excited about duplicating the
> pubname.

It’s indeed a bit silly to duplicate the information like that.

Another option would perhaps be to move to a format like the one in \dx+, and
only list the tables per subscription like how extension objects are listed.
Not sure if that’s any better here though.

cheers ./daniel

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

Re: [HACKERS] pg_receivewal and messages printed in non-verbose mode

[Daniel Gustafsson]

> On 14 Jun 2017, at 06:04, Michael Paquier  wrote:
> 
> On Tue, Jun 13, 2017 at 4:50 PM, Craig Ringer  wrote:
>> On 13 June 2017 at 14:33, Michael Paquier  wrote:
>>> Those come from stop_streaming in pg_receivewal.c. Shouldn't those
>>> messages only show up to the user if --verbose is used? It seems
>>> strange to me that at least the first one is written to the user as
>>> that's not an error after promoting a standby.
>> 
>> I agree. At least the first should be --verbose only.
> 
> I have been looking at all the code surrounding pg_receivewal and
> pg_recvlogical and those are indeed the two only places where we print
> a message in non-verbose mode even if those are not explicit errors.
> pg_recvlogical does not show up any messages when it is signaled or
> when it receives SIGINT or reaches the end of LSN position. I don't
> think that this is worth complicating the code for, just noticed the
> inconsistency on the way.
> 
> Perhaps a committer will care about that. Or not. For now I am just
> adding that in the CF.

+1, this patch makes --verbose behave consistently across these programs.
Since Ctrl-C’ing the program to terminate is not an error, I agree that it
should require verbose mode too.  I took a look as well and concur with your
findings.  Moving to Ready for Committer.

cheers ./daniel

-- 
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] psql's \d and \dt are sending their complaints to different output files

[Daniel Gustafsson]

> On 19 Jun 2017, at 17:32, Tom Lane  wrote:
> 
> So, if we're getting into enforcing consistency in describe.c, there's
> lots to do.
> 
> * listDbRoleSettings does this for a server too old to have the desired
> feature:
> 
>   fprintf(pset.queryFout,
>   _("No per-database role settings support in this server 
> version.\n"));
> 
> Just about every other function handles too-old-server like this:
> 
>   psql_error("The server (version %s) does not support full text 
> search.\n”,

Addressed in attached patch, see list of patches below.

> * listTables and listDbRoleSettings do this for zero matches:
> 
>   if (PQntuples(res) == 0 && !pset.quiet)
>   {
>   if (pattern)
>   fprintf(pset.queryFout, _("No matching relations 
> found.\n"));
>   else
>   fprintf(pset.queryFout, _("No relations found.\n"));
>   }
>   else
>   ... print the result normally
> 
> Note the test on quiet mode, as well as the choice of two messages
> depending on whether the command had a pattern argument or not.
> 
> Other functions in describe.c mostly follow this pattern:
> 
>   if (PQntuples(res) == 0)
>   {
>   if (!pset.quiet)
>   psql_error("Did not find any relation named \"%s\".\n",
>  pattern);
>   PQclear(res);
>   return false;
>   }
> 
> So this has a different behavior in quiet mode, which is to print
> *nothing* to queryFout rather than a result table with zero entries.
> That's probably bogus.  

There are two cases in verbose metacommands, we either print a generic “List of
XXX” title with a table following it, or multiple tables (with additional
non-table data) a per-table title.  For unmatched commands in the former case,
we print the title and an empty table in verbose mode, the latter case prints a
“Did not found XXX” message.  When QUIET is set to on, the latter case doesn’t
print anything for the most case.

Not printing anything is clearly not helpful, but choosing what to print
requires some thinking so before hacking on that I figured I’d solicit
opinions.  We can either keep printing a “Did not find ..” message; change a
per-table title to a generic one and include an empty table; a mix as today;
something completely different.  What preferences on output are there?

Personally I’m in favour of trying to add an empty table to all verbose
commands, simply because that’s what I expect to see when using psql.

> It will also crash, on some machines, if pattern
> is NULL, although no doubt nobody's noticed because there would always be
> a match.  (One call site does defend itself against null pattern, and it
> uses two messages like the previous example.)

Addressed in attached patch, see list of patches below.

> So we've got at least three things to normalize:
> 
> * fprintf(pset.queryFout) vs. psql_error()
> 
> * message wording inconsistencies
> 
> * behavior with -q and no matches.
> 
> Anybody want to draft a patch?

I poked around the code with an eye to improving consistency, and included some
more things that caught my eye.  Rather than attaching one patch with
everything, I pulled out the various proposals as separate patches:

0001 - Not really a consistency thing but included here since it’s sort of
related.  A small memleak on pattern2 spotted while reading code, unless I’m
missing where it’s freed.

0002 - While on the topic of consistency, tiny function comment reformat on the
metacommand function because I have comment-OCD, feel free to ignore.

0003 - Apply the same server version check pattern in listDbRoleSettings() as
elsewhere in other functions

0004 - Most verbose metacommands include additional information on top of what
is in the normal output, while \dRp+ dropped two columns (moving one to the
title).  This include the information from \dRp in \dRp+.  Having the pubname
in the title as well as the table is perhaps superfuous, but consistent with
other commands so opted for it.

0005 - Most tables titles were created using a PQExpBuffer with two using a
char buffer and snprintf().  Moved to using a PQExpBuffer instead in all cases
since it’s consistent and clean (not that the buffers risked overflowing or
anything like that, but I like the PQExpBuffer API).

0006 - Moves to psql_error() for all not-found messages and the same language
for all messages.  I don’t think these are errors per se, but psql_error() was
the most prevelant option used, so went there for consistency as a starting
point for discussion.  Also adds appropriate NULL deref check on pattern and
adds a not-found message in describePublications() which previously didn’t
print anything at all on not-found.

Hope there is something of interest in there.

cheers ./daniel



0001-Free-allocated-memory-when-2-patterns-used.patch
Description: Binary data

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 22 Jun 2017, at 17:52, Dilip Kumar <dilipbal...@gmail.com> wrote:
> 
> On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <dan...@yesql.se> wrote:
>> Good point.  I’ve attached a new version which issues a NOTICE on truncation
>> and also addresses all other comments so far in this thread.  Thanks a lot 
>> for
>> the early patch reviews!
>> 
>> cheers ./daniel
> 
> I have done an initial review of the patch. I have some comments/suggestions.

Thanks!

> +int
> +GetCancelMessage(char **buffer, size_t buf_len)
> +{
> + volatile BackendCancelShmemStruct *slot = MyCancelSlot;
> + int msg_length = 0;
> +
> 
> Returned value of this function is never used, better to convert it to
> just void.

You’re probably right, I was thinking that someone might be interested in
knowing about truncation when extracting the message but I can’t really think
of a callsite which wouldn’t just pass in a large enough buffer in the first
place.

> +bool
> +HasCancelMessage(void)
> +{
> + volatile BackendCancelShmemStruct *slot = MyCancelSlot;
> 
> +/*
> + * Return the configured cancellation message and its length. If the returned
> + * length is greater than the size of the passed buffer, truncation has been
> + * performed. The message is cleared on reading.
> + */
> +int
> +GetCancelMessage(char **buffer, size_t buf_len)
> 
> I think it will be good to merge these two function where
> GetCancelMessage will first check whether it has the message or not
> if it does then allocate the buffer of size slot->len and copy the
> slot message to allocated buffer otherwise just return NULL.
> 
> So it will look like "char *GetCancelMessage()”

It doesn’t seem like a good idea to perform memory allocation inside a spinlock
in a signalled backend, that would probably at least require an LWLock wouldn’t
it?  It seems safer to leave memory management to the signalled backend but it
may be paranoia on my part.

> + SpinLockAcquire(>mutex);
> + strlcpy(*buffer, (const char *) slot->message, buf_len);
> 
> strlcpy(*buffer, (const char *) slot->message, slot->len);
> 
> Isn't it better to copy only upto slot->len, seems like it will always
> be <= buf_len, and if not then
> we can do min(buf_len, slot->len)

strlcpy(3) is defined as taking the size of the passed buffer and not the
copied string.  Since we guarantee that slot->message is NUL terminated it
seems wise to stick to the API. Or did I misunderstand your comment?

cheers ./daniel

-- 
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] Multiple TO version in ALTER EXTENSION UPDATE

[Daniel Gustafsson]

> On 22 Jun 2017, at 17:02, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Robert Haas <robertmh...@gmail.com> writes:
>> On Wed, Mar 29, 2017 at 11:13 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
>>> While reading I noticed that we allow multiple TO  in ALTER 
>>> EXTENSION
>>> UPDATE, and defer throwing a syntax error until command processing.  Is 
>>> there a
>>> reason for deferring and not handling it in gram.y directly as in the 
>>> attached
>>> patch since it is in fact a syntax error?  It yields a different error 
>>> message
>>> to the user, but makes for easier to read code (IMH-and-biased-O).
> 
>> I think the idea of the current implementation was probably that the
>> grammar should leave room to support multiple options in arbitrary
>> order at that point in the syntax.  I'm not sure whether that's
>> something we'll ever actually need, or not.
> 
> It certainly seems plausible to me that we might someday grow additional
> options to control the UPDATE,

Fair enough, I was mainly curious about the reasoning, future proofing support
for additional options makes perfect sense.

> so I'm inclined to reject this patch.

I completely agree, I was using the patch to illustrate my question but wasn’t
very clear about that.

Thanks!

cheers ./daniel

-- 
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] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 22 Jun 2017, at 10:24, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> 
> On Thu, 22 Jun 2017 09:24:54 +0900
> Michael Paquier <michael.paqu...@gmail.com> wrote:
> 
>> On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <dan...@yesql.se> wrote:
>>> The message is truncated in SetBackendCancelMessage() for safety, but
>>> pg_{cancel|terminate}_backend() could throw an error on too long message, or
>>> warning truncation, to the caller as well.  Personally I think a warning is 
>>> the
>>> appropriate response, but I don’t really have a strong opinion.
>> 
>> And a NOTICE? That's what happens for relation name truncation. You
>> are right that having a check in SetBackendCancelMessage() makes the
>> most sense as bgworkers could just call the low level API. Isn't the
>> concept actually closer to just a backend message? This slot could be
>> used for other purposes than cancellation.
> 
> +1 for NOTICE. The message truncation seems to be a kind of helpful
> information rather than a likely problem as long as pg_terminated_backend
> exits successfully.
> 
> https://www.postgresql.org/docs/10/static/runtime-config-logging.html#runtime-config-severity-levels

Good point.  I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread.  Thanks a lot for
the early patch reviews!

cheers ./daniel



terminate_msg_v3.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] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 21 Jun 2017, at 16:30, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> 
> On Wed, 21 Jun 2017 12:06:33 +0900
> Michael Paquier <michael.paqu...@gmail.com> wrote:
> 
>> On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
>>> The message is stored in a new shmem area which is checked when the session 
>>> is
>>> aborted.  To keep things simple a small buffer is kept per backend for the
>>> message.  If deemed too costly, keeping a central buffer from which slabs 
>>> are
>>> allocated can be done (but seemed rather complicated for little gain 
>>> compared
>>> to the quite moderate memory spend.)
>> 
>> I think that you are right to take the approach with a per-backend
>> slot. This will avoid complications related to entry removals and
>> locking issues. There would be scaling issues as well if things get
>> very signaled for a lot of backends.
>> 
>> +#define MAX_CANCEL_MSG 128
>> That looks enough.
>> 
>> +   LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
>> +
>> +   strlcpy(slot->message, message, sizeof(slot->message));
>> +   slot->len = strlen(message);
>> Why not using one spin lock per slot and save it in BackendCancelShmemStruct?
> 
> +1
> 
> I found an example that a spin lock is used during strlcpy in 
> WalReceiverMain().

Yeah I agree as well, will fix.

>> +   pid_t   pid = PG_GETARG_INT32(0);
>> +   char   *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
>> +
>> +   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
>> It would be more solid to add some error handling for messages that
>> are too long, or at least truncate the message if too long.
> 
> I agree that error handling for too long messages is needed.
> Although long messages are truncated in SetBackendCancelMessage(),
> it is better to inform users that the message they can read was
> truncated one. Or, maybe we should prohibit too long message
> is passed in pg_teminate_backend()

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well.  Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

cheers ./daniel

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

[HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped.  This has nagged me in the
past and now it happened to come up again, so I took a stab at this.  The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

  SELECT pg_terminate_backend( [, message]);
  SELECT pg_cancel_backend( [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

  select pg_terminate_backend(, 'server rebooting');

..leads to:

  FATAL:  terminating connection due to administrator command: "server 
rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted.  To keep things simple a small buffer is kept per backend for the
message.  If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.) 

cheers ./daniel



terminate_msg_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

[HACKERS] Typos in comments

[Daniel Gustafsson]

Spotted one “paramter” typo and git grep found two more, patch attached with
s/paramter/parameter/ for these.

cheers ./daniel



typo-paramter.patch
Description: Binary data

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

[HACKERS] Typo in comment in ecpg datetime.c

[Daniel Gustafsson]

Spotted s/fiedls/fields/ in src/interfaces/ecpg/pgtypeslib/datetime.c per the
attached patch.

cheers ./daniel



typo-ecpg_datetime.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: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Daniel Gustafsson]

> On 30 May 2017, at 16:50, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
>>  wrote:
>>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>>> small flag about the stability and future of those APIs.
> 
>> It seems to me that the question is not just whether those APIs will
>> be available in future versions of OpenSSL, but whether they will be
>> available in every current and future version of every SSL
>> implementation that we may wish to use in core or that any client may
>> wish to use.  We've talked before about being able to use the Windows
>> native SSL implementation rather than OpenSSL and it seems that there
>> would be significant advantages in having that capability.
> 
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
> 
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Big +1.  The few settings we have already make it hard to provide other
implementations as drop-in replacements (Secure Transport doesn’t support
.crl files for example, only CRL loaded in Keychains).

cheers ./daniel

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Daniel Gustafsson]

> On 30 May 2017, at 18:25, Michael Paquier  wrote:
> 
> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

The Secure Transport documentation unfortunately excel at being incomplete and
hard to search in.  That being said, I think you’re right, AFAICT there is no
published API for this (SSLProcessFinished() seems like the best match but is
not public).

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

+1

cheers ./daniel

-- 
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] Bug when dumping "empty" operator classes

[Daniel Gustafsson]

> On 26 May 2017, at 17:08, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <dan...@yesql.se> writes:
>> While hacking on pg_upgrade in downstream Greenplum I ran into an error which
>> seems like an old, and obscure, bug in pg_dump (unrelated to pg_upgrade).
>> pg_dump generates incorrect SQL for an operator class which has no operators 
>> or
>> procedures, and which has the same column and storage types.
> 
> Good catch.
> 
>> The attached patch adds a belts-and-suspenders check in dumpOpclass() which
>> appends the STORAGE clause in case nothing had been added.
> 
> Seems reasonable (the comment could use some wordsmithing maybe) ...
> 
>> ... The DROP in
>> alter_generic is also removed to exercise the code path, being able to
>> pg_upgrade what is executed in regression seem like a good idea.
> 
> ... but that's a nonstarter.  We can't have the regression tests leaving
> global objects (users) lying around.

Fair enough, 

> I'll commit and back-patch this without a test case.  Possibly Frost will
> be excited enough about it to add something to the pg_dump TAP tests,
> but those tests are too opaque for me to want to do so.

Thanks!

cheers ./daniel

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

[HACKERS] Bug when dumping "empty" operator classes

[Daniel Gustafsson]

While hacking on pg_upgrade in downstream Greenplum I ran into an error which
seems like an old, and obscure, bug in pg_dump (unrelated to pg_upgrade).
pg_dump generates incorrect SQL for an operator class which has no operators or
procedures, and which has the same column and storage types.  While it’s
arguably a pretty uninteresting operator class, it is allowed since we don’t
validate that all required operators/procedures are present.

The alter_generic test suite contains examples of opclasses which trigger this
behavior.  The below operator class:

  CREATE OPERATOR CLASS alt_opc1 FOR TYPE uuid USING hash AS STORAGE uuid;

..is dumped like this (after an ALTER .. RENAME, thus the new name):

  CREATE OPERATOR CLASS alt_opc3
  FOR TYPE uuid USING hash FAMILY alt_opc1 AS
  ;

The reason why this hasn’t been caught by the PostgreSQL pg_upgrade test is
because the schema in which the operator classes are created is dropped at the
end of the suite, removing the DROP cause pg_upgrade to fail with:

  pg_restore: [archiver (db)] could not execute query: ERROR:  syntax error at 
or near ";"
  LINE 3: ;
  ^
  Command was: CREATE OPERATOR CLASS "alt_opc2"
  FOR TYPE "macaddr" USING "hash" FAMILY "alt_opc2" AS
  ;

The attached patch adds a belts-and-suspenders check in dumpOpclass() which
appends the STORAGE clause in case nothing had been added.  While adding
storage when it’s identical to the column type goes against the documentation,
it’s valid SQL and won’t break restore (and is handled by DefineOpClass()).
Validating the options fully would of course ensure that the dump always has
enough to render, but it also adds a lot of complexity (a quick look in the
archives doesn’t turn up many reports of it being a big problem).  The DROP in
alter_generic is also removed to exercise the code path, being able to
pg_upgrade what is executed in regression seem like a good idea.

cheers ./daniel



opclass_pgdump.patch
Description: Binary data

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

[HACKERS] Typo in json.c

[Daniel Gustafsson]

Spotted while reading code, patch attached.

cheers ./daniel



typo-json.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] A note about debugging TAP failures

[Daniel Gustafsson]

> On 24 Apr 2017, at 17:19, Craig Ringer <craig.rin...@2ndquadrant.com> wrote:
> 
> On 24 April 2017 at 20:01, Daniel Gustafsson <dan...@yesql.se> wrote:
> 
>> I’m np Perl expert though so there might be better/cleaner ways to achieve
>> this, in testing it seems to work though.  rmtree() is supported at least 
>> since
>> Perl 5.6 from what I can see.
> 
> I'd rather just have the 'make' target nuke the relevant tmp_check dir
> before rerunning the tests. Right now it just deletes the logs.

Makes sense, assuming a “clean slate” to run on seems a reasonable assumption
for the test and it makes for simpler code in PostgresNode.  Updated the patch
which now have static datadir names; retains on PG_TESTS_NOCLEAN or any kind of
test failure; and allows for cleaning datadirs without having to use make clean
(seems the wrong size hammer for that nail).

cheers ./daniel



tap_retaindir_v4.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] A note about debugging TAP failures

[Daniel Gustafsson]

> On 23 Apr 2017, at 15:05, Craig Ringer <craig.rin...@2ndquadrant.com> wrote:
> 
> On 23 Apr. 2017 10:32, "Michael Paquier" <michael.paqu...@gmail.com 
> <mailto:michael.paqu...@gmail.com>> wrote:
> On Sun, Apr 23, 2017 at 7:48 AM, Daniel Gustafsson <dan...@yesql.se 
> <mailto:dan...@yesql.se>> wrote:
> > Skipping the tempdir and instead using ${testname}_data_${name} without a
> > random suffix, we can achieve this with something along the lines of the
> > attached PoC.  It works as now (retain of failure, remove on success unless
> > overridden) but that logic can easily be turned around if we want that.  If
> > it’s of interest I can pursue this after some sleep (tomorrow has become 
> > today
> > at this point).
> 
> Yes, something like that may make sense as well for readability.
> 
> Keeping folders in case of failures is something that I have been
> advocating in favor of for some time, but this never got into the tree
> :(
> 
> Huh? We do keep test temp datadirs etc in case of failure. Just not on 
> success.
> 
> Our definition of failure there sucks a bit though. We keep the datadirs if 
> any test fails in a script. If the script its self crashes we still blow away 
> the datadirs which is kind of counterintuitive.
> 
> I'd like to change the __DIE__ sig handler to only delete on clean script 
> exit code, tap reporting success, and if some env bar like PG_TESTS_NOCLEAN 
> is undefined. The later could also be used in pg_regress etc.

That sounds like a good idea, even though END might be enough when not using
tempdir() for the datadirs since die() should set a non-zero $?  afaik (still
doesn’t hurt to capture with __DIE__ though).  I extended my previous test with
this, and other comments in this thread: it produces non-random datadirs,
retains them on die|exit|test-failure or if PG_TESTS_NOCLEAN is set.  Theres
also a check-clean target for cleaning out retained datadirs.

Given that the datadirs do occupy quite some space, perhaps a PG_TESTS_DOCLEAN
(or similar) would be good as well to always blow away the datadirs regardless
of test status?

I’m np Perl expert though so there might be better/cleaner ways to achieve
this, in testing it seems to work though.  rmtree() is supported at least since
Perl 5.6 from what I can see.

cheers ./daniel



tap_retaindir_v3.diff
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] A note about debugging TAP failures

[Daniel Gustafsson]

> On 23 Apr 2017, at 00:06, Andres Freund <and...@anarazel.de> wrote:
> 
> On 2017-04-22 23:59:11 +0200, Daniel Gustafsson wrote:
>> Since we have the name of the running testscript, can’t we just add that to 
>> the
>> tempdir to make the name more descriptive?  With the attached patch I get
>> tmp_check/001_pgbench_data_main_ItEm when running tests in src/bin/pgbench 
>> for
>> example which gives a decent clue to what was executed.  To allow for
>> retain-on-success, checking for an environment variable in the cleanup phase
>> seems the simplest approach.
> 
> Because it means we'd still, by default, have to delete on
> failure. Otherwise you'd end up with an endless number of such partial
> directories.  There's really no reason to do delete a failed directory
> ever, since the space usage is obviously bound.
> 
> As there appears to be no benefit in the randomness of these
> directories, why work on retaining it?

I’ve never managed a buildfarm animal so there might be something there I’m
missing, but other than that I can’t see much reason (running concurrent make
check's in the same directory doesn’t seem useful).  If one wants to keep a
directory around it’s easy enough to manually rename it.

Skipping the tempdir and instead using ${testname}_data_${name} without a
random suffix, we can achieve this with something along the lines of the
attached PoC.  It works as now (retain of failure, remove on success unless
overridden) but that logic can easily be turned around if we want that.  If
it’s of interest I can pursue this after some sleep (tomorrow has become today
at this point).

cheers ./daniel



tap_retaindir_v2.diff
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] A note about debugging TAP failures

[Daniel Gustafsson]

> On 22 Apr 2017, at 23:25, Tom Lane  wrote:
> 
> Andres Freund  writes:
>> On 2017-04-22 16:22:59 -0400, Tom Lane wrote:
>>> In the particular case I was interested in here, the test script thought
>>> everything was successful :-(.  I'm working on fixing that little problem,
>>> but I do not believe that the TAP scripts are so bulletproof that there
>>> will never be a need to override their judgment.
> 
>> Agreed.  If paths are reproducible and cleaned up on next run it's also
>> much less of an issue to just leave them around till the next run.
>> Which we imo also should do for non-failing tmp_check directories.
> 
> Mm, not convinced.  I think that the delete-on-success behavior was
> intentional, to limit the amount of disk space a buildfarm member would
> consume during a run.  I don't mind that being the default, as long as
> there's a way to override it manually.

Since we have the name of the running testscript, can’t we just add that to the
tempdir to make the name more descriptive?  With the attached patch I get
tmp_check/001_pgbench_data_main_ItEm when running tests in src/bin/pgbench for
example which gives a decent clue to what was executed.  To allow for
retain-on-success, checking for an environment variable in the cleanup phase
seems the simplest approach.

cheers ./daniel



tap_retaindir.diff
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] Old versions of Test::More

[Daniel Gustafsson]

> On 21 Apr 2017, at 15:08, Andrew Dunstan  
> wrote:
> 
> As we discovered yesterday via jacana, very old versions of Test::More
> don't support the note() function. It therefore seems prudent to specify
> a minimum version number for the module in those scripts that use the
> function. According to the changelog, version 0.82 (released in Oct
> 2008) should be sufficient. So I suggest the attached patch.

+1 for specifying version (0.82 was replaced with 0.84 within hours but it
doesn’t really matter for this).  However, since src/test/ssl/ServerSetup.pm
also invoke note() doesn’t it make sense to add the version requirement there
as well as a guard in case a testscript using ServerSetup isn’t explicitly
specifying the Test::More version?

cheers ./daniel



test-more-version-v2.diff
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