Re: [HACKERS] pgbench more operators & functions

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 5:28 PM, Fabien COELHO  wrote:

>
> Hello Haribabu,
>
> Alas, performance testing is quite sensitive to many details:-(
>>>
>>
> The current status of the patch and recent mail thread discussion doesn't
>> represent the same.
>>
>
> The same what?
>
> The discussion was about a particular test in a particular setting for a
> particular load, the fact that reducing the latency has a limited effect in
> that case is a fact in life. I have produced other settings where the
> effect was very important. The patch has no down side AFAICS.
>
> Closed in 2016-11 commitfest with "returned with feedback" status.
>> Please feel free to update the status once you submit the updated patch.
>>
>
> Given the thread discussions, I do not understand why this "ready for
> committer" patch is switched to "return with feedback", as there is nothing
> actionnable, and I've done everything required to improve the syntax and
> implementation, and to justify why these functions are useful.
>
> I'm spending time to try to make something useful of pgbench, which
> require a bunch of patches that work together to improve it for new use
> case, including not being limited to the current set of operators.
>
> This decision is both illogical and arbitrary.
>

Sorry for the changing the status of the patch against to the current
status.
While going through the recent mails, I thought that there is some
disagreement
from committer. Thanks for the clarification.

Updated status as follows.

Moved to next CF with "ready for committer" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Hash Indexes

[Amit Kapila]

On Thu, Dec 1, 2016 at 8:56 PM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 12:43 AM, Amit Kapila  wrote:
>> On Thu, Dec 1, 2016 at 2:18 AM, Robert Haas  wrote:
>>> On Wed, Nov 23, 2016 at 8:50 AM, Amit Kapila  
>>> wrote:
 [ new patch ]
>>>
>>> Committed with some further cosmetic changes.
>>
>> Thank you very much.
>>
>>> I think it would be worth testing this code with very long overflow
>>> chains by hacking the fill factor up to 1000
>>
>> 1000 is not a valid value for fill factor. Do you intend to say 100?
>
> No.  IIUC, 100 would mean split when the average bucket contains 1
> page worth of tuples.
>

I also think so.

>  I want to split when the average bucket
> contains 10 pages worth of tuples.
>

oh, I think what you mean to say is hack the code to bump fill factor
and then test it.  I was confused that how can user can do that from
SQL command.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] PATCH: two slab-like memory allocators

[Haribabu Kommi]

On Thu, Dec 1, 2016 at 1:26 PM, Tomas Vondra 
wrote:

>
>
> Dne 11/27/2016 v 11:02 PM Andres Freund napsal(a):
>
>> On 2016-11-27 22:21:49 +0100, Petr Jelinek wrote:
>>
>>> On 27/11/16 21:47, Andres Freund wrote:
>>>
 Hi,

 +typedef struct SlabBlockData *SlabBlock;   /* forward
>> reference */
>> +typedef struct SlabChunkData *SlabChunk;
>>
>> Can we please not continue hiding pointers behind typedefs? It's a bad
>> pattern, and that it's fairly widely used isn't a good excuse to
>> introduce further usages of it.
>>
>>
> Why is it a bad pattern?
>

 It hides what is passed by reference, and what by value, and it makes it
 a guessing game whether you need -> or . since you don't know whether
 it's a pointer or the actual object. All to save a * in parameter and
 variable declaration?...


>>> FWIW I don't like that pattern either although it's used in many
>>> parts of our code-base.
>>>
>>
>> But relatively few new ones, most of it is pretty old.
>>
>>
> I do agree it's not particularly pretty pattern, but in this case it's
> fairly isolated in the mmgr sources, and I quite value the consistency in
> this part of the code (i.e. that aset.c, slab.c and generation.c all use
> the same approach). So I haven't changed this.
>
> The attached v7 fixes the off-by-one error in slab.c, causing failures in
> test_decoding isolation tests, and renames Gen to Generation, as proposed
> by Petr.
>

Moved to next CF with same status (needs review).

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'

[Haribabu Kommi]

On Thu, Nov 17, 2016 at 12:57 PM, Haribabu Kommi 
wrote:

> Hi Dmitry,
>
> This is a gentle reminder.
>
> you assigned as reviewer to the current patch in the 11-2016 commitfest.
> But you haven't shared your review yet. Can you please try to share your
> views
> about the patch. This will help us in smoother operation of commitfest.
>
> some people are against to the current patch approach. If you can share
> your
> views on the use case and etc, it will be helpful. If you are not agreed
> with the
> approach similar like others, better reject the patch.
>
> Please Ignore if you already shared your review.
>
>
Closed in 2016-11 commitfest with "rejected" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Danger of automatic connection reset in psql

[Haribabu Kommi]

On Tue, Nov 22, 2016 at 11:06 PM, Pavel Stehule 
wrote:

>
>
> 2016-11-22 13:02 GMT+01:00 Oleksandr Shulgin  >:
>
>> On Tue, Nov 22, 2016 at 5:28 AM, Pavel Stehule 
>> wrote:
>>
>>>
>>> 2016-11-22 3:46 GMT+01:00 Robert Haas :
>>>
 On Mon, Nov 21, 2016 at 4:55 AM, Oleksandr Shulgin
  wrote:
 > On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby 
 wrote:
 >>
 >> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
 >>>
 >>> Automatic connection reset is a nice feature for server development,
 >>> IMO.  Is it really useful for anything else is a good question.
 >>
 >>
 >> I use it all the time for application development; my rebuild script
 will
 >> forcibly kick everyone out to re-create the database. I put that in
 because
 >> I invariably end up with a random psql sitting somewhere that I
 don't want
 >> to track down.
 >>
 >> What currently stinks though is if the connection is dead and the
 next
 >> command I run is a \i, psql just dies instead of re-connecting. It'd
 be nice
 >> if before reading the script it checked connection status and
 attempted a
 >> reconnect.
 >>
 >>> At least an option to control that behavior seems like a good idea,
 >>> maybe even set it to 'no reconnect' by default, so that people who
 >>> really use it can make conscious choice about enabling it in their
 >>> .psqlrc or elsewhere.
 >>
 >>
 >> +1, I don't think it needs to be the default.
 >
 >
 > So if we go in this direction, should the option be specified from
 command
 > line or available via psqlrc (or both?)  I think both make sense.
 >
 > What should be the option and control variable names?  Something like:
 > --reconnect and RECONNECT?  Should we allow reconnect in
 non-interactive
 > mode?  I have no use case for that, but it might be different for
 others.
 > If non-interactive is not supported then it could be a simple boolean
 > variable, otherwise we might want something like a tri-state: on /
 off /
 > interactive (the last one being the default).

 I think it should just be another psql special variable, like
 AUTOCOMMIT or VERBOSITY.  If the user wants to set it on the command
 line, they can just use -v.  We don't need a separate, dedicated
 option for this, I think.

>>>
>> That makes sense to me.
>>
>> In this case depends on a default. For almost all scripts the sensible
>>> value is "without reconnect". It be unfriendly to use this setting via -v
>>> variable.
>>>
>>
>> Well, if you're running a script it should not be affected as long as
>> default value for this new variable is "interactive" or "off" (and you
>> didn't override it in psqlrc).  If you really want to get a "reconnect even
>> from the script" type of behavior, then you'll have to use -v or set the
>> variable from inside the script itself to "on".
>>
>
> ok
>
>
Closed in 2016-11 commitfest with "returned with feedback" status.
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] proposal: psql \setfileref

[Haribabu Kommi]

On Fri, Nov 18, 2016 at 5:53 AM, Pavel Stehule 
wrote:

> Hi
>
> 2016-11-17 12:00 GMT+01:00 Pavel Stehule :
>
>>
>> Hi
>>
>> a independent implementation of parametrized queries can looks like
>> attached patch:
>>
>> this code is simple without any unexpected behave.
>>
>> parameters are used when user doesn't require escaping and when
>> PARAMETRIZED_QUERIES variable is on
>>
>> Regards
>>
>
> here is a Daniel's syntax patch
>
> The independent implementation of using query parameters is good idea, the
> patch looks well, and there is a agreement with Tom.
>
> I implemented a Daniel proposal - it is few lines, but personally I prefer
> curly bracket syntax against `` syntax. When separator is double char, then
> the correct implementation will not be simple - the bad combinations "` ``,
> `` `" should be handled better. I wrote my arguments for my design, but if
> these arguments are not important for any other, then I can accept any
> other designs.
>
>
The recent updated patch as per the agreement hasn't received any feedback
from reviewers. Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pgbench - allow backslash continuations in \set expressions

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 6:13 AM, Tom Lane  wrote:

> Fabien COELHO  writes:
> >> In psql, if backslash followed by [CR]LF is interpreted as a
> >> continuation symbol, commands like these seem problematic
> >> on Windows since backslash is the directory separator:
> >>
> >> \cd \
> >> \cd c:\somepath\
> >>
> >> Shell invocations also come to mind:
> >> \! dir \
>
> > Thanks for pointing out these particular cases. I was afraid of such
> > potential issues, hence my questions...
>
> Those look like nasty counterexamples, but I think they are not, because
> they don't work today.  What you get is
>
> regression=# \cd \
> Invalid command \. Try \? for help.
>
> AFAICT you would need to write it
>
> regression=# \cd '\\'
> \cd: could not change directory to "\": No such file or directory
>
> (That's on Unix of course, on Windows I'd expect it to succeed.)
>
> The reason for this is that psql already has a policy that an unquoted
> backslash begins a new backslash command on the same line.  Since
> there is no command named backslash-return, this is available syntax
> that can be repurposed in the direction we want.
>
> I believe that we need to do basically the same thing in pgbench, and
> I'm fine with that because I think we should have an overall policy of
> synchronizing the psql and pgbench metacommand syntaxes as best we can.
> This will at some point necessitate invention of a quoting rule for
> pgbench metacommand arguments, so that you can write a backslash as part
> of an argument when you need to.  We might not need to do that today
> (as I'm not sure there are any use-cases for one), but maybe it'd be best
> to just bite the bullet and put it in.
>
>
The review from the committer is arrived at the end of commitfest,
so moved the patch into next CF with "waiting on author" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pgbench - allow to store select results into variables

[Haribabu Kommi]

On Mon, Oct 3, 2016 at 12:43 AM, Michael Paquier 
wrote:

> On Tue, Sep 27, 2016 at 10:41 AM, Amit Langote
>  wrote:
> > On 2016/09/26 20:27, Fabien COELHO wrote:
> >>
> >> Hello Amit,
> >>
>  I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as
> I
>  have no further comments at the moment.
> >>>
> >>> Wait... Heikki's latest commit now requires this patch to be rebased.
> >>
> >> Indeed. Here is the rebased version, which still get through my various
> >> tests.
> >
> > Thanks, Fabien.  It seems to work here too.
>
> Moved to next CF with same status, ready for committer.
>

Moved to next CF with same status (ready for committer).

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] [PATCH] pgpassfile connection option

[Haribabu Kommi]

On Tue, Nov 29, 2016 at 2:53 AM, Fabien COELHO  wrote:

>
> Hello Julian,
>
> I've adressed those spacing errors.
>>
>
> Ok.
>
> You are right, if pgpassfile_used is true, it SHOULD be defined, I just
>> like to be careful whenever I'm working with strings. But I guess in this
>> scenario I can trust the caller and omit those checks.
>>
>
> Good.
>
> Patch looks ok, applies, compiles & checks, and tested manually.
>
> I've switch in the CF to "ready for committer", and we'll see what the
> next level thinks about it:-)
>
> [...] I agree with those criticisms of the multi-host feature and
>> notifying the client in case of an authentification error rather than
>> trying other hosts seems sensible to me.
>>
>
> Sure. I complained about the fuzzy documentation & imprecise warning
> message because I stumbled upon that while testing.
>
> But I think fixes for those should be part of different patches, as this
>> patch's aim was only to expand the existing pgpassfile functionality to be
>> used with a parameter.
>>
>
> Yes.
>
>
Moved to next commitfest with same status (ready for committer).


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] patch: function xmltable

[Haribabu Kommi]

On Thu, Dec 1, 2016 at 2:21 AM, Pavel Stehule 
wrote:

> Dne 30. 11. 2016 14:53 napsal uživatel "Pavel Stehule" <
> pavel.steh...@gmail.com>:
> >
> >
> >
> > 2016-11-30 13:38 GMT+01:00 Alvaro Herrera :
> >>
> >> Pavel Stehule wrote:
> >> > 2016-11-30 2:40 GMT+01:00 Craig Ringer :
> >> >
> >> > > On 30 November 2016 at 05:36, Pavel Stehule <
> pavel.steh...@gmail.com>
> >> > > wrote:
> >> > >
> >> > > > The problem is in unreserved keyword "PASSING" probably.
> >> > >
> >> > > Yeah, I think that's what I hit when trying to change it.
> >> > >
> >> > > Can't you just parenthesize the expression to use operators like ||
> >> > > etc? If so, not a big deal.
> >> > >
> >> > ???
> >>
> >> "'(' a_expr ')'" is a c_expr; Craig suggests that we can just tell users
> >> to manually add parens around any expressions that they want to use.
> >> That's not necessary most of the time since we've been able to use
> >> b_expr in most places.
> >
>
> still there are one c_expr, but without new reserved word there are not
> change to reduce it.
>
>
>
Moved to next CF with the same status (ready for committer).

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pg_sequence catalog

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 1:47 PM, Andreas Karlsson  wrote:

> I think this patch looks good now so I am setting it to ready for
> committer.
>
> I like the idea of the patch and I think that while this change will break
> some tools which look at the sequence relations I think the advantages are
> worth it (for example making more sequence DDL respecting MVCC).
>

Moved to next CF with the same status (ready for committer).

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pgbench more operators & functions

[Fabien COELHO]

Hello Haribabu,


Alas, performance testing is quite sensitive to many details:-(



The current status of the patch and recent mail thread discussion doesn't
represent the same.


The same what?

The discussion was about a particular test in a particular setting for a 
particular load, the fact that reducing the latency has a limited effect 
in that case is a fact in life. I have produced other settings where the 
effect was very important. The patch has no down side AFAICS.



Closed in 2016-11 commitfest with "returned with feedback" status.
Please feel free to update the status once you submit the updated patch.


Given the thread discussions, I do not understand why this "ready for 
committer" patch is switched to "return with feedback", as there is 
nothing actionnable, and I've done everything required to improve the 
syntax and implementation, and to justify why these functions are useful.


I'm spending time to try to make something useful of pgbench, which 
require a bunch of patches that work together to improve it for new use 
case, including not being limited to the current set of operators.


This decision is both illogical and arbitrary.

--
Fabien.


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

Re: [HACKERS] Parallel execution and prepared statements

[Amit Kapila]

On Thu, Dec 1, 2016 at 9:40 PM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 7:57 AM, Amit Kapila  wrote:
>>
>> I have tried to restrict all the non-readonly operation modes or modes
>> where parallelism might not make sense like DestTupleStore.  If we
>> want to just prohibit the cases where it can fail now, then I think
>> prohibiting only DestIntoRel should be sufficient because that is a
>> case where the user is allowed to do DDL for an already prepared read
>> only statement like Create Table AS .. EXECUTE.
>
> OK, then my vote is to do it that way for now.
>

Done that way in attached patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


prepared_stmt_parallel_query_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] pgbench more operators & functions

[Haribabu Kommi]

On Sat, Oct 8, 2016 at 11:27 PM, Fabien COELHO  wrote:

>
> Hello Amit.
>
> Also, given the heavy UPDATE nature of the pgbench test, a non 100% default
>>> fill factor on some tables would make sense.
>>>
>>
>> FWIW, sometime back I have seen that with fill factor 80, at somewhat
>> moderate client counts (32) on 192 - Hyper Threaded m/c, the
>> performance is 20~30% better, but at higher client counts, it was same
>> as 100 fill factor.
>>
>
> The 20-30% figure is consistent with figures I collected 2 years ago about
> fill factor on HDD, see the beginning run of:
>
> http://blog.coelho.net/database/2014/08/23/postgresql-
> fillfactor-and-update.html
>
> Although I found that the advantages is reduced after some time because
> once a page has got an update it has some free space which can be taken
> advantage of later on, if the space was not reclaimed by vacuum.
>
> I cannot understand why there would be no advantage with more clients,
> though...
>
> Alas, performance testing is quite sensitive to many details:-(
>
>
The current status of the patch and recent mail thread discussion doesn't
represent the same.

Closed in 2016-11 commitfest with "returned with feedback" status.
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] LOCK TABLE .. DEFERRABLE

[Simon Riggs]

On 15 September 2016 at 18:51, Robert Haas  wrote:
> On Tue, Sep 6, 2016 at 6:04 AM, Simon Riggs  wrote:
>> On 1 September 2016 at 21:28, Simon Riggs  wrote:
>>> So the only way to handle multiple locks is to do this roughly the way
>>> Rod suggests.
>>
>> I've just been reading the VACUUM code and it turns out that we
>> already use Rod's mechanism internally. So on that basis it seems fine
>> to support this as a useful user-level feature. If there is a better
>> way of doing it, then that can be added later.
>>
>> My proposed changes to this patch are these
>>
>> 1. Rename this WAIT PATIENTLY, which is IMHO a better description of
>> what is being requested. Bikeshedding welcome.
>>
>> 2. Introduce a new API call LockTablePatiently() that returns bool. So
>> its usage is similar to ConditionalLockTable(), the only difference is
>> you supply some other wait parameters with it. This isolates the
>> internal mechanism from the usage, so allows us to more easily support
>> any fancy new way of doing this we think of later.
>>
>> 3. Use LockTablePatiently() within lockcmds.c where appropriate
>>
>> 4. Replace the code for waiting in VACUUM with the new call to
>> LockTablePatiently()
>>
>> So I see this as 2 patches: 1) new API and make VACUUM use new API, 2)
>> Rod's LOCK TABLE patch
>>
>> First patch attached, requires also lock by Oid.  If we agree, Rod,
>> please update your patch to match?
>
> Aside from the fact that polling is generally inefficient and wasteful
> of system resources, this allows for undetected deadlocks.  Consider:
>
> S1: LOCK TABLE A;
> S2: LOCK TABLE B;
> S1: LOCK TABLE B; -- blocks
> S2: LOCK TABLE A PATIENTLY; -- retries forever

Hmmm

> Livelock might be possible, too.
>
> I think it would be better to think harder about what would be
> required to implement this natively in the lock manager.  Suppose we
> add a flag to each PROCLOCK which, if true, indicates that the lock
> request is low priority.  Also, we add a counter to each LOCK
> indicating the number of pending low-priority lock requests.  When
> LockAcquireExtended reaches this code here...
>
> if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
> status = STATUS_FOUND;
> else
> status = LockCheckConflicts(lockMethodTable, lockmode,
>
>  lock, proclock);
>
> ...we add an additional check to the upper branch: if the number of
> low-priority waiters is not 0, then we walk the wait queue; if all
> waiters that conflict with the requested lock mode are low-priority,
> then we set status = STATUS_OK.  So, new lock requests refuse to queue
> behind low-priority lock waiters.

Well, that's pretty much the exact design I mentioned earlier.

> Is that good enough to implement the requested behavior, or do we need
> to do more?

The only problem is that Rod's request was to be able to lock multiple
tables in one statement, which cannot then be done that way.

But there are problems with Rod's approach, so I suggest we ditch that
now and I'll implement the single table lock approach that you, me and
Andres preferred.

I'd rather have something sweet with one table than something crappy
with multiple tables.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Random number generation, take two

[Michael Paquier]

On Wed, Nov 30, 2016 at 10:22 PM, Michael Paquier
 wrote:
> On Wed, Nov 30, 2016 at 8:51 PM, Heikki Linnakangas  wrote:
>> On 11/30/2016 09:01 AM, Michael Paquier wrote:
>>> +bool
>>> +pg_backend_random(char *dst, int len)
>>> +{
>>> +   int i;
>>> +   char   *end = dst + len;
>>> +
>>> +   /* should not be called in postmaster */
>>> +   Assert (IsUnderPostmaster || !IsPostmasterEnvironment);
>>> +
>>> +   LWLockAcquire(BackendRandomLock, LW_EXCLUSIVE);
>>> Shouldn't an exclusive lock be taken only when the initialization
>>> phase is called? When reading the value a shared lock would be fine.
>
> Do we need to worry about performance in the case of application doing
> small transactions and creating new connections for each transaction?
> This would become a contention point when calculating cancel keys for
> newly-forked backends. It could be rather easy to measure a
> concurrency impact with for example pgbench -C with many concurrent
> transactions running something as light as SELECT 1.

I got curious about this point, so I have done a couple of tests with
my laptop using the following pgbench command:
pgbench -f test.sql -C -c 128 -j 4 -t 100
And test.sql is just that:
\set aid random(1,10)
In short, a backend is spawned and a cancel key is generated but
nothing is done with it to avoid any overhead. With HEAD and the patch
without/with --disable-strong-random, I am seeing pretty close
numbers. My laptop has only 4 cores, so we may see something on a
machine with a higher number of cores. But as far as things go I am in
the noise range.
-- 
Michael


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

Re: [HACKERS] asynchronous and vectorized execution

[Haribabu Kommi]

On Mon, Oct 3, 2016 at 3:25 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> At Mon, 3 Oct 2016 13:14:23 +0900, Michael Paquier <
> michael.paqu...@gmail.com> wrote in  xn_g7uwnpqun_a_sewb...@mail.gmail.com>
> > On Thu, Sep 29, 2016 at 5:50 PM, Kyotaro HORIGUCHI
> >  wrote:
> > > Sorry for no response, but, the answer is yes. We could be able
> > > to avoid the problem by managing execution state for every
> > > node. But it needs an additional flag in *State structs and
> > > manipulating on the way shuttling up and down around the
> > > execution tree.
> >
> > Moved to next CF.
>
> Thank you.



Closed in 2016-11 commitfest with "returned with feedback" status.
This is as per my understanding of the recent mails on the thread.
Please feel free to update the status if the current status doesn't
reflect the exact status of the patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] WAL logging problem in 9.4.3?

[Haribabu Kommi]

On Wed, Nov 9, 2016 at 5:55 PM, Michael Paquier 
wrote:

>
>
> On Wed, Nov 9, 2016 at 9:27 AM, Michael Paquier 
> wrote:
> > On Wed, Nov 9, 2016 at 5:39 AM, Robert Haas 
> wrote:
> >> On Thu, Feb 4, 2016 at 7:24 AM, Heikki Linnakangas 
> wrote:
> >>> I dropped the ball on this one back in July, so here's an attempt to
> revive
> >>> this thread.
> >>>
> >>> I spent some time fixing the remaining issues with the prototype patch
> I
> >>> posted earlier, and rebased that on top of current git master. See
> attached.
> >>>
> >>> Some review of that would be nice. If there are no major issues with
> it, I'm
> >>> going to create backpatchable versions of this for 9.4 and below.
> >>
> >> Are you going to do commit something here?  This thread and patch are
> >> now 14 months old, which is a long time to make people wait for a bug
> >> fix.  The status in the CF is "Ready for Committer" although I am not
> >> sure if that's accurate.
> >
> > "Needs Review" is definitely a better definition of its current state.
> > The last time I had a look at this patch I thought that it was in
> > pretty good shape (not Horiguchi-san's version, but the one in
> > https://www.postgresql.org/message-id/CAB7nPqR+3JjS=JB3R=
> axxkxcyeb-q77u-erw7_ukajctwnt...@mail.gmail.com).
> > With some of the recent changes, surely it needs a second look, things
> > related to heap handling tend to rot quickly.
> >
> > I'll look into it once again by the end of this week if Heikki does
> > not show up, the rest will be on him I am afraid...
>
> I have been able to hit a crash with recovery test 008:
> (lldb) bt
> * thread #1: tid = 0x, 0x7fff96d48f06 
> libsystem_kernel.dylib`__pthread_kill
> + 10, stop reason = signal SIGSTOP
>   * frame #0: 0x7fff96d48f06 libsystem_kernel.dylib`__pthread_kill +
> 10
> frame #1: 0x7fff9102e4ec libsystem_pthread.dylib`pthread_kill + 90
> frame #2: 0x7fff8e5cc6df libsystem_c.dylib`abort + 129
> frame #3: 0x000106ef10f0 
> postgres`ExceptionalCondition(conditionName="!((
> !( ((void) ((bool) (! (!((buffer) <= NBuffers && (buffer) >= -NLocBuffer))
> || (ExceptionalCondition(\"!((buffer) <= NBuffers && (buffer) >=
> -NLocBuffer)\", (\"FailedAssertion\"), \"bufmgr.c\", 2593), 0, (buffer)
> != 0 ) ? ((bool) 0) : ((buffer) < 0) ? (LocalRefCount[-(buffer) - 1] > 0) :
> (GetPrivateRefCount(buffer) > 0) ))", errorType="FailedAssertion",
> fileName="bufmgr.c", lineNumber=2593) + 128 at assert.c:54
> frame #4: 0x000106cf4a2c postgres`BufferGetBlockNumber(buffer=0)
> + 204 at bufmgr.c:2593
> frame #5: 0x00010694e6ad postgres`HeapNeedsWAL(rel=0x7f9454804118,
> buf=0) + 61 at heapam.c:9234
> frame #6: 0x00010696d8bd 
> postgres`visibilitymap_set(rel=0x7f9454804118,
> heapBlk=1, heapBuf=0, recptr=50841176, vmBuf=118, cutoff_xid=866,
> flags='\x01') + 989 at visibilitymap.c:310
> frame #7: 0x00010695d020 
> postgres`heap_xlog_visible(record=0x7f94520035d0)
> + 896 at heapam.c:8148
> frame #8: 0x00010695c582 
> postgres`heap2_redo(record=0x7f94520035d0)
> + 242 at heapam.c:9107
> frame #9: 0x0001069d132d postgres`StartupXLOG + 9181 at xlog.c:6950
> frame #10: 0x000106c9d783 postgres`StartupProcessMain + 339 at
> startup.c:216
> frame #11: 0x0001069ee6ec postgres`AuxiliaryProcessMain(argc=2,
> argv=0x7fff59316d80) + 1676 at bootstrap.c:420
> frame #12: 0x000106c98002 
> postgres`StartChildProcess(type=StartupProcess)
> + 322 at postmaster.c:5221
> frame #13: 0x000106c96031 postgres`PostmasterMain(argc=3,
> argv=0x7f9451c04210) + 6033 at postmaster.c:1301
> frame #14: 0x000106bc30cf postgres`main(argc=3,
> argv=0x7f9451c04210) + 751 at main.c:228
> (lldb) up 1
> frame #4: 0x000106cf4a2c postgres`BufferGetBlockNumber(buffer=0) +
> 204 at bufmgr.c:2593
>2590{
>2591BufferDesc *bufHdr;
>2592
> -> 2593Assert(BufferIsPinned(buffer));
>2594
>2595if (BufferIsLocal(buffer))
>2596bufHdr = GetLocalBufferDescriptor(-buffer - 1);
>

The latest proposed patch still having problems.
Closed in 2016-11 commitfest with "moved to next CF" status because of a
bug fix patch.
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pg_xlogdump follow into the future

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 2:21 AM, Vladimir Rusinov 
wrote:

> This patch does not have a reviewer, so I've decided to try myself on.
>
> Disclaimer: although I review quite a lot of code daily, this is my first
> review for PostgreSQL. I don't know code very well, and frankly I don't
> really know C very well.
> Hope my effort are not vain and will be helpful to somebody.
> I'll be happy for review on review and any tips on process.
>
> Summary
> ===
>
> I favour this patch. Current behaviour is indeed confusing. If we keep
> current behaviour we need to update docs and maybe also print a warning
> when using -f with a file name.
>
> Thank you for submission, but i'm afraid there is a bit more work here:
>
> - There is a bug, making it hard to test. Please fix.
> - Please add some tests.
>
> Submission review
> ==
>
> Patch applies cleanly on HEAD. Tests succeed.
> There are no new or affected by this patch tests. Given that I've found a
> trivial bug (see below), a test should be created.
>
> Usability review
> 
> I believe I've immediately hit a corner case:
>
> 1) I've created a new instance, started it and run `./bin/pg_xlogdump -f
> db/pg_wal/00010001`.
> This spewed quite a lot of stuff, as expected.
>
> 2) I've connected to the same instance and ran following:
> # SELECT pg_switch_xlog();
>  pg_switch_xlog
> 
>  0/14FA3D8
> (1 row)
>
> xlogdump immediately crashed with following:
>
> pg_xlogdump: FATAL:  could not find file "00010002": No
> such file or directory
>
> Problem is that Postgres does not create file until it's actually needed.
> So 00010002 indeed was not there until after I've run some
> transactions. I believe same would happen after pg_start_backup(), etc.
>
> Feature review
> ===
> See above. Can't do more testing.
>
> Performance review
> ===
> n/a
>
> Coding review
> ===
> LGTM
>
> Architecture review
> ==
> n/a
>

Patch received feedback at the end of commitfest.
Closed in 2016-11 commitfest with "moved to next CF".
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] VACUUM's ancillary tasks

[Haribabu Kommi]

On Wed, Oct 19, 2016 at 2:44 AM, Robert Haas  wrote:

> On Sun, Oct 16, 2016 at 3:35 PM, Jeff Janes  wrote:
> > On Fri, Oct 7, 2016 at 6:14 AM, Robert Haas 
> wrote:
> >> On Thu, Oct 6, 2016 at 8:40 PM, Jeff Janes 
> wrote:
> >> > In commit 37484ad2aacef5ec7, you changed the way that frozen tuples
> were
> >> > represented, so that we could make freezing more aggressive without
> >> > losing
> >> > forensic evidence.  But I don't think we ever did anything to actually
> >> > make
> >> > the freezing more aggressive.
> >>
> >> See 3cff1879f8d03cb729368722ca823a4bf74c0cac.  The objection to doing
> >> it in other cases is that it adds write-ahead log volume which might
> >> cause its own share of problems.  There might be some way of getting
> >> ahead of that, though.  I think if we piggyback on an existing WAL
> >> record like XLOG_HEAP2_CLEAN or XLOG_HEAP2_VISIBLE the impact might be
> >> minimal, but I haven't been dedicated enough to try to write the
> >> patch.
> >>
> >> > When I applied the up-thread patch so that pgbench_history gets
> autovac,
> >> > those autovacs don't actually cause any pages to get frozen until the
> >> > wrap
> >> > around kicks in, even when all the tuples on the early pages should be
> >> > well
> >> > beyond vacuum_freeze_min_age.
> >>
> >> If the pages are already all-visible, they'll be skipped until
> >> vacuum_freeze_table_age is reached.
> >
> > So if I set vacuum_freeze_min_age to zero, then they should become
> > all-visible and all-frozen at the same time, and avoid that problem?
>
> Hmm.  I *think* so...
>
> > From the docs on vacuum_freeze_min_age: "Increasing this setting may
> avoid
> > unnecessary work if the rows that would otherwise be frozen will soon be
> > modified again".  How much work is that? Presumably they are already
> getting
> > marked visible, is marking them frozen too a meaningful amount of extra
> > work?  Is it just the extra WAL record?
>
> Yeah, the extra WAL record is the main thing, I think.


Closed in 2016-11 commitfest with "returned with feedback" status as per my
understanding from the recent mails on the thread.

Please feel free to change the status if the current status is doesn't
reflect the
actual status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Wed, Nov 30, 2016 at 7:53 PM, Amit Kapila  wrote:
> On Mon, Nov 21, 2016 at 11:08 AM, Michael Paquier
>  wrote:
>> On Tue, Nov 15, 2016 at 9:59 PM, Amit Kapila  wrote:
>>> On Tue, Nov 15, 2016 at 9:27 AM, Kyotaro HORIGUCHI
>>>  wrote:
 The term "WAL activity' is used in the comment for
 GetProgressRecPtr. Its meaning is not clear but not well
 defined. Might need a bit detailed explanation about that or "WAL
 activity tracking". What do you think about this?

>>>
>>> I would have written it as below:
>>>
>>> GetProgressRecPtr -- Returns the WAL progress.  WAL progress is
>>> determined by scanning each WALinsertion lock by taking directly the
>>> light-weight lock associated to it.
>>
>> Not sure if that's better.. What about something as fancy as that?
>>  /*
>> - * Get the time of the last xlog segment switch
>> + * GetProgressRecPtr -- Returns the newest WAL progress position.  WAL
>> + * progress is determined by scanning each WALinsertion lock by taking
>> + * directly the light-weight lock associated to it.  The result of this
>> + * routine can be compared with the last checkpoint LSN to check if
>> + * a checkpoint can be skipped or not.
>> + *
>> It may be worth mentioning that the result of this routine is
>> basically used for checkpoint skip logic.
>>
>
> That's okay, but I think you are using it to skip switch segment stuff
> as well.  Today, again going through patch, I noticed small anomaly
>
>> + * Switch segment only when WAL has done some progress since the
> + * > last time a segment has switched because of a timeout.
>
>> + if (GetProgressRecPtr() > last_switch_lsn)
>
> Either the above comment is wrong or the code after it has a problem.
> last_switch_lsn aka XLogCtl->lastSegSwitchLSN is updated not only for
> a timeout but also when there is a lot of WAL activity which makes WAL
> Write to cross a segment boundary.

Right, this should be reworded a bit better to mention both. Done as attached.
-- 
Michael


hs-checkpoints-v19.patch
Description: invalid/octet-stream

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

Re: [HACKERS] LOCK TABLE .. DEFERRABLE

[Haribabu Kommi]

On Fri, Sep 16, 2016 at 3:51 AM, Robert Haas  wrote:

> On Tue, Sep 6, 2016 at 6:04 AM, Simon Riggs  wrote:
> > On 1 September 2016 at 21:28, Simon Riggs  wrote:
> >> So the only way to handle multiple locks is to do this roughly the way
> >> Rod suggests.
> >
> > I've just been reading the VACUUM code and it turns out that we
> > already use Rod's mechanism internally. So on that basis it seems fine
> > to support this as a useful user-level feature. If there is a better
> > way of doing it, then that can be added later.
> >
> > My proposed changes to this patch are these
> >
> > 1. Rename this WAIT PATIENTLY, which is IMHO a better description of
> > what is being requested. Bikeshedding welcome.
> >
> > 2. Introduce a new API call LockTablePatiently() that returns bool. So
> > its usage is similar to ConditionalLockTable(), the only difference is
> > you supply some other wait parameters with it. This isolates the
> > internal mechanism from the usage, so allows us to more easily support
> > any fancy new way of doing this we think of later.
> >
> > 3. Use LockTablePatiently() within lockcmds.c where appropriate
> >
> > 4. Replace the code for waiting in VACUUM with the new call to
> > LockTablePatiently()
> >
> > So I see this as 2 patches: 1) new API and make VACUUM use new API, 2)
> > Rod's LOCK TABLE patch
> >
> > First patch attached, requires also lock by Oid.  If we agree, Rod,
> > please update your patch to match?
>
> Aside from the fact that polling is generally inefficient and wasteful
> of system resources, this allows for undetected deadlocks.  Consider:
>
> S1: LOCK TABLE A;
> S2: LOCK TABLE B;
> S1: LOCK TABLE B; -- blocks
> S2: LOCK TABLE A PATIENTLY; -- retries forever
>
> Livelock might be possible, too.
>
> I think it would be better to think harder about what would be
> required to implement this natively in the lock manager.  Suppose we
> add a flag to each PROCLOCK which, if true, indicates that the lock
> request is low priority.  Also, we add a counter to each LOCK
> indicating the number of pending low-priority lock requests.  When
> LockAcquireExtended reaches this code here...
>
> if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
> status = STATUS_FOUND;
> else
> status = LockCheckConflicts(lockMethodTable, lockmode,
>
>  lock, proclock);
>
> ...we add an additional check to the upper branch: if the number of
> low-priority waiters is not 0, then we walk the wait queue; if all
> waiters that conflict with the requested lock mode are low-priority,
> then we set status = STATUS_OK.  So, new lock requests refuse to queue
> behind low-priority lock waiters.
>
> Is that good enough to implement the requested behavior, or do we need
> to do more?  If we only do what's described above, then a
> normal-priority waiter which joins the queue after a low-priority
> waiter is already waiting will let the low-priority waiter go first.
> That's probably not desirable, but it's pretty easy to fix: the logic
> that determines where a new waiter enters the wait queue is in
> ProcSleep(), and it wouldn't be too hard to arrange for new
> normal-priority waiters to skip over any low-priority waiters that are
> at the end of the existing wait queue (but not any that are in the
> middle, because if we did that we'd also be skipping over
> normal-priority waiters, which we shouldn't).
>
> What more?  Well, even after doing those two things, it's still
> possible for either the simple deadlock logic in ProcSleep() or the
> full deadlock detector to put a low-priority waiter in front of a
> normal-priority waiter.  However, our typical policy is to advance
> waiters in the wait queue as little as possible.  In other words, if
> the wait queue contains A B C and we will deadlock unless C is moved
> up, we will move it ahead of B but not A if that is sufficient to
> avoid the deadlock.  We will only move it ahead of both B and A if
> that is necessary to avoid deadlock.  So, low-priority requests won't
> be moved up further than needed, which is good.
>
> Still, it is possible to construct scenarios where we won't get
> perfect low-priority behavior without more invasive changes. For
> example, suppose we make a low-priority request queue-jump over an
> intervening waiter to avoid deadlocking against it.  Next, a
> normal-priority waiter enters the queue.  Then, the guy we skipped
> aborts.  At this point, we could in theory notice that it's possible
> to move the low-priority request behind the new normal-priority
> waiter.  However, I think we shouldn't do that.  We certainly can't do
> it unconditionally because it might introduce deadlocks.  We could
> test whether it will introduce a deadlock and do it only if not, but
> that's expensive.  Instead, I think we should document that a
> low-priority request will ordinarily cause the request to be satisfied
> only after all 

Re: [HACKERS] WIP: Covering + unique indexes.

[Haribabu Kommi]

On Sat, Nov 19, 2016 at 8:38 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 10/4/16 10:47 AM, Anastasia Lubennikova wrote:
> > 03.10.2016 15:29, Anastasia Lubennikova:
> >> 03.10.2016 05:22, Michael Paquier:
> >>> On Tue, Sep 27, 2016 at 12:17 AM, Anastasia Lubennikova
> >>>  wrote:
>  Ok, I'll write it in a few days.
> >>> Marked as returned with feedback per last emails exchanged.
> >>
> >> The only complaint about this patch was a lack of README,
> >> which is fixed now (see the attachment). So, I added it to new CF,
> >> marked as ready for committer.
> >
> > One more fix for pg_upgrade.
>
> Latest patch doesn't apply.  See also review by Brad DeJong.  I'm
> setting it back to Waiting.


Closed in 2016-11 commitfest with "returned with feedback" status.
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Fast Default WIP patch for discussion

[Haribabu Kommi]

On Sat, Oct 29, 2016 at 2:28 AM, Serge Rielau  wrote:

> Time for me to dig into that then.
>

Closed in 2016-11 commitfest with "returned with feedback" status.
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

[Alvaro Herrera]

I have now pushed this to 9.5, 9.6 and master.  It could be backpatched
to 9.4 with ease (just a small change in heap_form_tuple); anything
further back would require much more effort.

I used a 32-bit limit using sizeof(int32).  I tested and all the
mentioned cases seem to work sanely; if you can spare some more time to
test what was committed, I'd appreciate it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Michael Paquier]

On Fri, Dec 2, 2016 at 11:26 AM, Andreas Karlsson  wrote:
> I have attached a new version. The commitfest should technically have been
> closed by now, so do what you like with the status. I can always submit the
> patch to the next commitfest.

I have just moved it to the next CF. Will look at it in couple of
days, that's more or less at the top of my TODO list.
-- 
Michael


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Thu, Dec 1, 2016 at 9:31 PM, Michael Paquier
 wrote:
> On Thu, Dec 01, 2016 at 03:17:34PM -0500, Robert Haas wrote:
>> It might be that (as suggested downthread) we should consider
>> supporting multiple IPs in the hostaddr string as well, but that
>> requires some thought.  For example, what happens if, for example, the
>> host and hostaddr lists are of unequal length?  Would we accept one
>> host and >1 hostaddrs?  Probably makes sense to just apply the host to
>> every hostaddr.  >1 host and 1 hostaddr?  Probably doesn't make sense,
>> but I guess you could argue for it.  Equal length lists definitely
>> make sense.
>
> That would make the current code a huge plate of spagetthi for sanity
> checks considering the multiple interations between port, host and
> hostaddr. It seems to me that the current approach of supporting only
> port and host is simple enough and will satisfy most of the user's
> need plently. So +1 for simplicity.

I don't think it'd be ridiculously complicated to make it work and I
don't mind if someone wants to try.  However, it wasn't interesting to
me, so I didn't spend time on it.  A lot of these parameters are
intertwined, and I wanted to avoid trying to boil the ocean.  But I'm
not allergic to follow-on patches.

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


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

Re: [HACKERS] Broken SSL tests in master

[Mithun Cy]

On Fri, Dec 2, 2016 at 2:26 AM, Robert Haas  wrote:
>Yeah, we should change that.  Are you going to write a patch?

Thanks, will work on this will produce a patch to patch to fix.

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

Re: [HACKERS] PassDownLimitBound for ForeignScan/CustomScan [take-2]

[Haribabu Kommi]

On Thu, Nov 10, 2016 at 10:59 AM, Kouhei Kaigai 
wrote:

> > On Tue, Nov 8, 2016 at 6:54 AM, Jeevan Chalke
> >  wrote:
> > > 1. ps_numTuples is declared as long, however offset and count members
> in
> > > LimitState struct and bound member in SortState struct is int64.
> However
> > > long on 32 bit machine may be 32 bits and thus I think tuples_needed
> which
> > > is long may have overflow hazards as it may store int64 + int64.  I
> think
> > > ps_numTuples should be int64.
> >
> > I suggested long originally because that's what ExecutorRun() was
> > using at the time.  It seems that it got changed to uint64 in
> > 23a27b039d94ba359286694831eafe03cd970eef, so I guess we should
> > probably use uint64.
> >
> > > 2. Robert suggested following in the previous discussion:
> > > "For example, suppose we add a new PlanState member "long
> > > numTuples" where 0 means that the number of tuples that will be needed
> > > is unknown (so that most node types need not initialize it), a
> > > positive value is an upper bound on the number of tuples that will be
> > > fetched, and -1 means that it is known for certain that we will need
> > > all of the tuples."
> > >
> > > We should have 0 for the default case so that we don't need to
> initialize it
> > > at most of the places.  But I see many such changes in the patch.  I
> think
> > > this is not possible here since 0 can be a legal user provided value
> which
> > > cannot be set as a default (default is all rows).
> > >
> > > However do you think, can we avoid that? Is there any other way so
> that we
> > > don't need every node having ps_numTuples to be set explicitly?
> >
> > +1.
> >
> I thought we have to distinguish a case if LIMIT 0 is supplied.
> However, in this case, ExecLimit() never goes down to the child nodes,
> thus, its ps_numTuples shall not be referenced anywhere.
>
> OK, I'll use uint64 for ps_numTuples, and 0 shall be a usual default
> value that means no specific number of rows are given.



Marked as "returned with feedback" in 2016-11 commitfest.
Please feel free to update the status when you submit the latest patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Add support for restrictive RLS policies

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 2:09 AM, Stephen Frost  wrote:

> Dean,
>
> * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> > On 1 December 2016 at 14:38, Stephen Frost  wrote:
> > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> > >> In get_policies_for_relation() ...
> > >> ... I think it should sort the restrictive policies by name
> > >
> > > Hmmm, is it really the case that the quals will always end up being
> > > evaluated in that order though?  Isn't order_qual_clauses() going to
> end
> > > up changing the order based on the relative cost?  If the cost is the
> > > same it should maintain the order, but even that could change in the
> > > future based on the comments, no?  In short, I'm not entirely sure that
> > > we actually want to be required to always evaluate the quals in order
> of
> > > policy name and we might get complaints if we happen to make that work
> > > today and it ends up being changed later.
> >
> > No, this isn't about the quals that get put into the WHERE clause of
> > the resulting queries. As you say, order_quals_clauses() is going to
> > re-order those anyway. This is about the WithCheckOption's that get
> > generated for UPDATEs and INSERTs, and having those checked in a
> > predictable order. The main advantage to that is to guarantee a
> > predictable error message from self tests that attempt to insert
> > invalid data. This is basically the same as what was done for CHECK
> > constraints in e5f455f59fed0632371cddacddd79895b148dc07.
>
> You know, you said that in your email, and I read it and it made sense
> to me, and then I went off to do something else and came back and
> completely forgot that you were referring to the WITH CHECK case.
>
> You're right, we should order the WithCheckOptions.  I'll update the
> patch accordingly.
>


Moved to next CF with "waiting on author" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] postgres_fdw super user checks

[Haribabu Kommi]

On Tue, Oct 18, 2016 at 10:38 AM, Michael Paquier  wrote:

> On Mon, Oct 17, 2016 at 10:51 PM, Robert Haas 
> wrote:
> > On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier
> >  wrote:
> >> On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes 
> wrote:
> >>> postgres_fdw has some checks to enforce that non-superusers must
> connect to
> >>> the foreign server with a password-based method.  The reason for this
> is to
> >>> prevent the authentication to the foreign server from happening on the
> basis
> >>> of the OS user who is running the non-foreign server.
> >>>
> >>> But I think these super user checks should be run against the userid
> of the
> >>> USER MAPPING being used for the connection, not the userid of currently
> >>> logged on user.
> >>
> >> So, if the user mapping user is a superuser locally, this would allow
> >> any lambda user of the local server to attempt a connection to the
> >> remote server. It looks dangerous rather dangerous to me to authorize
> >> that, even if the current behavior is a bit inconsistent I agree.
> >
> > I don't know what "any lambda user" means.  Did you mean to write "any
> > random user"?
>
> Yes, in this context that would be "any non-superuser" or "any user
> without superuser rights". Actually that's a French-ism. I just
> translated it naturally to English to define a user that has no access
> to advanced features :)
>


Thanks for the patch, but it breaking the existing functionality as per the
other
mails. Marked as "returned with feedback" in 2016-11 commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Rename max_parallel_degree?

[Haribabu Kommi]

On Thu, Oct 27, 2016 at 3:38 AM, Robert Haas  wrote:

> On Mon, Oct 24, 2016 at 4:04 PM, Peter Eisentraut
>  wrote:
> > On 10/12/16 7:58 PM, Robert Haas wrote:
> >> I don't think it's wrong that the handling is done there, though.  The
> >> process that is registering the background worker is well-placed to
> >> check whether there are already too many, and if it does not then the
> >> slot is consumed at least temporarily even if it busts the cap.  On
> >> the flip side, the postmaster is the only process that is well-placed
> >> to know when a background worker terminates.  The worker process
> >> itself can't be made responsible for it, as you suggest below, because
> >> it may never even start up in the first place (e.g. fork() returns
> >> EAGAIN).  And the registering process can't be made responsible,
> >> because it might die before the worker.
> >
> > Those are valid technical points.  I have not worked out any
> alternatives.
> >
> > I'm concerned that all this makes background workers managed by
> > extensions second-class citizens.
>
> I suppose at some level that's true, but I have a hard time getting
> worked up about it.  There are always some areas where core code is
> going to be privileged over non-core code, but I'd say that background
> workers stand out as a shining example of enabling interesting
> non-core code.  In fact, the main reason why this patch exists at all
> is so that the sole in-core user of the background worker facility -
> parallel query - doesn't crowd out interesting non-core uses of that
> facility.  This isn't a parallel query feature; it's an
> everything-that-uses-background-workers-that-is-not-parallel-query
> feature.
>
> Also, for many types of background workers, this kind of limiting
> behavior isn't really useful or doesn't really make sense.  Obviously,
> any application that uses exactly one background worker (or any fixed
> number of background workers) doesn't need any feature similar to this
> one.  Also, any application that uses a "launcher" process and some
> number of per-database workers can limit the number of workers it
> consumes by teaching the launcher process to enforce a limit.  These
> two patterns are quite common, I think, and probably cover a lot of
> what background workers might like to do.  Parallel query is a little
> bit unusual in that any backend in the system might launch workers
> without having any idea what workers other backends are already doing.
> It seems unlikely to me that many extensions would share this pattern,
> but perhaps I lack sufficient imagination.  pg_background, or any
> facility derived from it, might qualify, but I can't think what else
> would.
>
> In any event, this is not written in stone.  I don't think it would be
> a huge deal to change this later if we come up with something better.
>


>From the recent mails, it is not clear to me what is the status of this
patch.
moved to next CF with "needs review" state. Please feel free to update
the status.

Regards,
Hari Babu
Fujitsu Australia

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

[Haribabu Kommi]

On Tue, Nov 15, 2016 at 5:58 PM, Haribabu Kommi 
wrote:

>
>
> On Sat, Nov 12, 2016 at 10:12 PM, Pavan Deolasee  > wrote:
>
>>
>>
>> On Tue, Nov 8, 2016 at 9:13 AM, Haribabu Kommi 
>> wrote:
>>
>>>
>>> Thanks for the patch. This shows a very good performance improvement.
>>>
>>>
>> Thank you. Can you please share the benchmark you ran, results and
>> observations?
>>
>
> I just ran a performance test on my laptop with minimal configuration, it
> didn't show much
> improvement, currently I don't have access to a big machine to test the
> performance.
>
>
>
>> I started reviewing the patch, during this process and I ran the
>>> regression
>>> test on the WARM patch. I observed a failure in create_index test.
>>> This may be a bug in code or expected that needs to be corrected.
>>>
>>
>> Can you please share the diff? I ran regression after applying patch on
>> the current master and did not find any change? Does it happen consistently?
>>
>
>
> Yes, it is happening consistently. I ran the make installcheck. Attached
> the regression.diffs file with the failed test.
> I applied the previous warm patch on this commit
> - e3e66d8a9813d22c2aa027d8f373a96d4d4c1b15
>


Are you able to reproduce the issue?

Currently the patch is moved to next CF with "needs review" state.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pg_recvlogical --endpos

[Haribabu Kommi]

On Wed, Nov 30, 2016 at 4:26 PM, Okano, Naoki 
wrote:

>
> On Wednesday, November 30, 2016 10:34 AM Craig Ringer wrote:
> >On 30 November 2016 at 09:18, Okano, Naoki 
> >
> wrote:
> >>
> >> On November 29, 2016 at 5:03 PM Craig Ringer wrote:
> >>> Would it be better rephrased as "--endpos can only be used with
> --start" ?
> >> OK. I think this phrase is better than the previous phrase.
> >>
>  The patch should allow --endpos to work with --create-slot.
> >>>
> >>> How? It doesn't make sense with --create-slot .
> >> This patch is updated to incorporate Alvaro's changes shown below,
> isn't it?
> >> https://www.postgresql.org/message-id/20160503180658.
> GA59498%40alvherre.pgsql
> >> I thought that the consent in community was taken with this
> specification...
> >>> I could not find any mention that it did not make sense with
> --create-slot.
>
> > What would --endpos with --create-slot do?
>
> Sorry, I was misunderstanding. you are right.
> I have noticed that --endpos doesn't make sense unless it is with --start.
> I checked --endpos works with --create-slot and --start.
> So, there is no problem with this patch.
>
>
Moved to next CF with "needs review" state.


Regards,
Hari Babu
Fujitsu Australia

[HACKERS] 答复: [HACKERS] Re: [HACKERS] 答复: [HACKERS] postgres 1 个(共 2 个) can pg 9.6 vacuum freeze skip page on index?

[xu jian]

Thanks every for your help. I am not familiar with the internal of the vacuum 
freeze, just curious if there is no row change on the table(in other words, all 
pages are frozen), why could index page have dead tuple?

is it possible to scan data page first, if all data page are frozen, skipping 
the index page scan step. Perhaps there is other reason vacuum freeze does 
index page first, then is it possible to provide a option to skip index page 
scan step in vacuum freeze command? thanks


James


发件人: Robert Haas 
发送时间: 2016年12月1日 13:50:49
收件人: Tom Lane
抄送: xu jian; Masahiko Sawada; pgsql-hackers@postgresql.org
主题: Re: [HACKERS] Re: [HACKERS] 答复: [HACKERS] postgres 1 个(共 2 个) can pg 9.6 
vacuum freeze skip page on index?

On Thu, Dec 1, 2016 at 1:39 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I think that the indexes only need to be scanned if the VACUUM finds
>> dead tuples.  But even 1 dead tuple will cause a complete scan of
>> every index.  I've complained about this before and I think there's
>> room for improvement here, but nobody's been motivated enough to
>> pursue this yet.
>
> The thing that's been speculated about in the past is having some
> threshold larger than 1 on the minimum number of dead tuples needed
> to cause a cleanup pass.

Agreed.

> It wouldn't be hard to implement, if you
> could get consensus on what the threshold should be.

Also agreed.

> I'd think
> some algorithm similar to the autovacuum thresholds might be
> appropriate.  It's not quite clear how this would interact with
> HOT pruning, though.

What's the relevance of HOT pruning here?

I was thinking that the relevant metric might be how many pages
contain dead tuples, because what we really want to do to reduce the
cost of future vacuuming and future index-only scans is get pages
marked all-visible.  Say, if less than 2% of the pages in the table
contain dead tuples and the space required to store the TIDs is less
than 50% of maintenance_work_mem, skip the index scans.  The first of
those thresholds, at least, would probably need to be configurable,
but that kind of idea.

The alternative that's been proposed is to do something based on the
number of dead tuples but, as somebody pointed out in a previous
discussion of this topic, one dead tuple per page throughout the whole
table is a LOT worse than same number of dead tuples all on the same
pages.  You don't want to keep scanning large chunks of the heap
because you're too lazy to visit the indexes.

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

Re: [HACKERS] pg_sequence catalog

[Andreas Karlsson]

I think this patch looks good now so I am setting it to ready for committer.

I like the idea of the patch and I think that while this change will 
break some tools which look at the sequence relations I think the 
advantages are worth it (for example making more sequence DDL respecting 
MVCC).


Andreas


--
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] DROP FUNCTION of multiple functions

[Peter Eisentraut]

On 11/23/16 5:04 PM, Tom Lane wrote:
> I looked at this briefly.  I agree that 0001-0003 are simple cleanup of
> the grammar and could be pushed without further ado.

Done.

> However, starting
> with 0004 I begin to get queasy.  The plan seems to be that instead of
> "objname" always being a List of strings, for functions (and then
> aggregates and operators) it will be a one-element List of some new struct
> that has then got a name list inside it.

I think the original idea of representing an object by a list of name
components plus optionally a list of arguments has accumulated too many
warts and should be replaced.  For example: A Typename isn't a list, so
it has to be packed into a List to be able to pass it around.  Some
objects only have a single-component string as a name.  For a cast, we
arbitrarily put the source type into a the name list and the target type
into the argument list.  For an operator class on the other hand we
create a cons of name and access method.  The pending logical
replication patch has more such arbitrary examples.  This pattern has to
be repeated consistently in gram.y for all cases where the object is
referenced (ALTER EXTENSION, DROP, COMMENT, RENAME, SET SCHEMA, OWNER
TO) and then consistently unpacked in objectaddress.c.

I think it would be better to get rid of objargs and have objname be a
general Node that can contain more specific node types so that there is
some amount of type tracking.  FuncWithArgs would be one such type,
Typename would be another, Value would be used for simple strings, and
we could create some other ones, or stick with lcons for some simple
cases.  But then we don't have to make stuff into one-item lists to just
to satisfy the currently required List.

That's the general idea.  But that's a rather big change that I would
rather break down into smaller pieces.  I have a separate patch in
progress for that, which I have attached here.  It breaks some
regression tests in object_address.sql, which I haven't evaluated yet,
but that's the idea.

However, in these current patches I just wanted to take the first step
to normalize the representation of functions so that actual
functionality changes could be built in top of that.

> It leads to code with random changes of data representation at seemingly
> random places, like this bit from 0005:
>  
> + if (stmt->removeType == OBJECT_AGGREGATE || stmt->removeType == 
> OBJECT_FUNCTION)
> + objname = list_make1(objname);

Yeah, the reason for that is that when we want to store something as
objname, it needs to be a list, and the convention is to wrap non-lists
into a single-member list.  So then objectaddress.c is coded to linitial
such lists when working on object types such as functions or types.
RemoveObjects() takes the list it gets from the grammar and passes each
element to get_object_address().  But a function_with_argtypes_list is a
list of FuncWithArgs, so we have to make those into single-member lists
first.  The reason this works for types is that type_name_list looks
like this:

type_name_list:
Typename   { $$ = list_make1(list_make1($1)); }
  | type_name_list ',' Typename{ $$ = lappend($1, list_make1($3)); }

I suppose we could make function_with_argtypes look like that as well
(and later change it back if we redesign it as discussed above).  I
think if we did it that way, it would get rid of the warts in this patch
set.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 3af0716745895bfe24b5e2e7ab1f0d4aaa3ec011 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 12 Nov 2016 12:00:00 -0500
Subject: [PATCH] Remove objargs

WIP
---
 src/backend/catalog/objectaddress.c |  92 +---
 src/backend/commands/alter.c|   9 ++-
 src/backend/commands/comment.c  |   4 +-
 src/backend/commands/dropcmds.c |  50 ++---
 src/backend/commands/extension.c|   4 +-
 src/backend/commands/seclabel.c |   4 +-
 src/backend/commands/tablecmds.c|   1 -
 src/backend/nodes/copyfuncs.c   |   8 ---
 src/backend/nodes/equalfuncs.c  |   8 ---
 src/backend/parser/gram.y   | 136 
 src/backend/parser/parse_utilcmd.c  |   2 -
 src/include/catalog/objectaddress.h |   6 +-
 src/include/nodes/parsenodes.h  |   8 ---
 13 files changed, 117 insertions(+), 215 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d531d17..58d04fa 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -684,11 +684,11 @@ static ObjectAddress get_object_address_type(ObjectType objtype,
 static ObjectAddress get_object_address_opcf(ObjectType objtype, List *objname,
 		bool missing_ok);
 static ObjectAddress get_object_address_opf_member(ObjectType objtype,
-			  List *objname, 

Re: [HACKERS] Broken SSL tests in master

[Michael Paquier]

On Thu, Dec 01, 2016 at 03:17:34PM -0500, Robert Haas wrote:
> It might be that (as suggested downthread) we should consider
> supporting multiple IPs in the hostaddr string as well, but that
> requires some thought.  For example, what happens if, for example, the
> host and hostaddr lists are of unequal length?  Would we accept one
> host and >1 hostaddrs?  Probably makes sense to just apply the host to
> every hostaddr.  >1 host and 1 hostaddr?  Probably doesn't make sense,
> but I guess you could argue for it.  Equal length lists definitely
> make sense.

That would make the current code a huge plate of spagetthi for sanity
checks considering the multiple interations between port, host and
hostaddr. It seems to me that the current approach of supporting only
port and host is simple enough and will satisfy most of the user's
need plently. So +1 for simplicity.
-- 
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Andreas Karlsson]

On 11/30/2016 06:52 AM, Michael Paquier wrote:

On Mon, Nov 28, 2016 at 2:01 PM, Michael Paquier

Looking at the latest patch at code-level, there is some refactoring
to introduce initialize_context(). But it is actually not necessary
(perhaps this is the remnant of a past version?) as be_tls_init() is
its only caller. This patch makes hard to look at the diffs, and it
makes future back-patching more complicated, so I would suggest
simplifying the patch as much as possible. You could use for example a
goto block for the error code path to free the context with
SSL_CTX_free(), and set up ssl_loaded_verify_locations once the CA is
loaded. The same applies to initialize_ecdh().

+   if (secure_initialize() != 0)
+   ereport(FATAL,
+   (errmsg("could not load ssl context")));
+   LoadedSSL = true;
In case of a failure, a LOG message would have been already generated,
so this duplicates the information. Worse, if log_min_messages is set
to a level higher than LOG, users *lose* information on what has
happened. I think that secure_initialize() should have an argument to
define elevel and that this routine should be in charge of generating
an error message. Having it return a status code is necessary, but you
could cast secure_initialize() with (void) to show that we don't care
about the status code in this case. There is no need to care about
freeing the new context when the FATAL code path is used as process
would just shut down.


Thanks, this is a really good suggestion which made the diff much 
cleaner. I removed my new macro too now since I felt it mostly made the 
code more cryptic just to gain a few lines of code.



config.sgml needs to be updated to reflect that the SSL parameters are
updated at server reload (mentioned already upthread, just
re-mentioning it to group all the comments in one place).


Thanks, fixed this.


As this patch could be really simplified this way, I am marking is as
returned with feedback.


I have attached a new version. The commitfest should technically have 
been closed by now, so do what you like with the status. I can always 
submit the patch to the next commitfest.


Andreas
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b1c5289..5f80930 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -959,9 +959,8 @@ include_dir 'conf.d'

 Enables SSL connections. Please read
  before using this. The default
-is off. This parameter can only be set at server
-start.  SSL communication is only possible with
-TCP/IP connections.
+is off. SSL communication is only
+possible with TCP/IP connections.

   
  
@@ -979,7 +978,7 @@ include_dir 'conf.d'
 and client certificate verification is not performed.  (In previous
 releases of PostgreSQL, the name of this file was hard-coded
 as root.crt.)  Relative paths are relative to the
-data directory.  This parameter can only be set at server start.
+data directory.

   
  
@@ -994,8 +993,7 @@ include_dir 'conf.d'

 Specifies the name of the file containing the SSL server certificate.
 The default is server.crt.  Relative paths are
-relative to the data directory.  This parameter can only be set at
-server start.
+relative to the data directory.

   
  
@@ -1012,8 +1010,7 @@ include_dir 'conf.d'
 revocation list (CRL).  The default is empty, meaning no CRL file is
 loaded.  (In previous releases of PostgreSQL, the name of this file was
 hard-coded as root.crl.)  Relative paths are
-relative to the data directory.  This parameter can only be set at
-server start.
+relative to the data directory.

   
  
@@ -1028,8 +1025,7 @@ include_dir 'conf.d'

 Specifies the name of the file containing the SSL server private key.
 The default is server.key.  Relative paths are
-relative to the data directory.  This parameter can only be set at
-server start.
+relative to the data directory.

   
  
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 787cfce..5e78d81 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2288,8 +2288,8 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
 The files server.key, server.crt,
 root.crt, and root.crl
 (or their configured alternative names)
-are only examined during server start; so you must restart
-the server for changes in them to take effect.
+are examined when reloading the configuration, or when spawning the backend
+process on Windows systems.

   
 
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 668f217..ebd00de 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ 

Re: [HACKERS] Stopping logical replication protocol

[Haribabu Kommi]

On Fri, Nov 4, 2016 at 12:44 AM, Craig Ringer  wrote:

> On 21 October 2016 at 19:38, Vladimir Gordiychuk  wrote:
> > Craig, Andres what do you thinks about previous message?
>
> I haven't had a chance to look further to be honest.
>
> Since a downstream disconnect works, though it's ugly, it's not
> something I can justify spending a lot of time on, and I already did
> spend a lot on it in patch review/updating/testing etc.
>
> I don't know what Andres wants, but I think CopyFail with
> ERRCODE_QUERY_CANCELED is fine.
>
> As for plugins that collect changes in memory and only send them on
> commit, I'd call them "broken". Not an interesting case IMO. Don't do
> that.
>

Currently the patch is marked as "returned with feedback" as there are some
comments from reviewer.

Please free to submit an update patch to the next commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] btree_gin and btree_gist for enums

[Haribabu Kommi]

On Wed, Nov 23, 2016 at 11:31 AM, Andrew Dunstan 
wrote:

> I won't have time to fix this before the end of the Commitfest


The patch is marked as "returned with feedback" in 2016-11 commitfest.
Please free to submit an updated patch to the next commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] s/xlog/wal/ in tools and function names?

[Michael Paquier]

On Fri, Dec 2, 2016 at 3:21 AM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 10:29 AM, Vladimir Rusinov  wrote:
>> I've found myself wondering "where is my xlog" after running
>> pg_switch_xlog() in 10.0.
>>
>> Renaming pg_xlog to pg_wal created inconsistency between tools, function
>> names and directory name on disk.
>>
>> Should we also:
>>
>> - rename pg_switch_xlog and friends to pg_switch_wal?
>> - rename pg_recievexlog to pg_revievewal (and others in bin/)?
>> - rename pg_xlogdump to pg_waldump?
>
> I think yes to all.

I was hesitant to propose that, but if there is a will do move
everything... Documentation would point to different pages if the
utilities are renamed, so that's not helpful when comparing features
across major releases... We may want to keep those files with their
historical names.

>> - if we do rename, should we keep aliases for functions and symlinks for
>> tools?
>
> I think no.

Better to do breakages in a single release rather than spreading them
across releases. While at it and because we are on a crazy trend, one
thing we could as well consider is removing pg_xlog_location_diff().
It has lost sense since pg_lsn has been introduced.

>> - anything else?
>
> There are some SQL-callable functions that should probably be renamed
> to match, too.

=# select proname from pg_proc where proname ~ 'xlog';
 proname
-
 pg_current_xlog_location
 pg_current_xlog_insert_location
 pg_current_xlog_flush_location
 pg_xlogfile_name_offset
 pg_xlogfile_name
 pg_xlog_location_diff
 pg_last_xlog_receive_location
 pg_last_xlog_replay_location
 pg_is_xlog_replay_paused
 pg_switch_xlog
 pg_xlog_replay_pause
 pg_xlog_replay_resume
(12 rows)
-- 
Michael


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

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Tom Lane]

Robert Haas  writes:
> Given the precedent in pgbench (cf.
> 878fdcb843e087cc1cdeadc987d6ef55202ddd04), I think it requires an
> amazing level of optimism to suppose we won't eventually end up with a
> full-blown expression language here.  I would suggest designing one
> from the beginning and getting it over with.  Even if you manage to
> hold the line and exclude it from whatever gets committed initially,
> somebody's going to propose it 2 years from now.  And again 4 years
> from now.  And again 2 years after that.

The other problem with not thinking about that general case is that
people will keep on proposing little bitty features that nibble at
the problem but may or may not be compatible with a general solution.
To the extent that such patches get accepted, we'll be forced into
either backwards-compatibility breakage or sub-optimal solutions when
we do get to the point of wanting a general answer.  I'd much rather
start with a generalized design and then implement it piece by piece.

(This is more or less the same point as my nearby stand against localized
hacking of backslash parsing rules.)

regards, tom lane


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

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

[Magnus Hagander]

On Fri, Dec 2, 2016 at 2:28 AM, Michael Paquier 
wrote:

> On Fri, Dec 2, 2016 at 10:10 AM, Robert Haas 
> wrote:
> > On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs 
> wrote:
> >> * pg_basebackup -R
> >> will write recovery.trigger to data directory
> >> insert parameters postgresql.conf.auto, if possible
> >
> > Don't understand that last line; otherwise, +1.
>
> pg_basebackup -R creates a recovery.conf now, by appending the
> parameters to postgresql.conf.auto we are sure that:
> 1) there is no need to check for the existence of recovery.conf as it
> could be included by postgresql.conf with something like an
> include_if_exists
> 2) postgresql.conf.auto is loaded automatically without any additional
> tweaks needed in the GUC parsing code paths.
>

I'd also add the point that once there, you can deal with it the same way
as other parameters with say ALTER SYSTEM if you wish, so it integrates
with existing processes better than creating a separate config file and
including it. And since postgresql.conf.auto has a very well defined
format, editing it by machine (pg_basebackup that is) is fairly simple and
safe.



> >> * Add docs: "Guide to changes in recovery.conf in 10.0"
> >
> > Hmm, we don't usually write the docs in terms of how things are
> > different from a previous version.  Might seem strange in 5 years.
> > Not sure what's best, here.
>
> A good chunk in the release notes would make sense as well?
>

It would.

And in fairness, having such a "guide to changes" chapter in each release
probably *would* be a good idea. But it would take resources to make that.
The release notes are good, but having a more hand-holding version
explaining incompatible changes in "regular sentences" would probably be
quite useful to users.



> >> * recovery_target as a single parameter, using proposed "xid 666"
> >> "target value" format
> >
> > +1.
> >
> >> * remove hot_standby parameter altogether, in line with earlier changes
> >
> > That seems a little surprising.  We don't think anyone ever wants to
> > refuse connections during archive recovery?


Sure. But pg_hba.conf does that, and listen_addresses does that. We don't
really need yet-another-parameter for it.



> I suggested that yesterday. We have talked as well about merging
> standby_mode with hot_standby, but at the end most use cases I have
> seen involve looking at pg_is_in_recovery() these days to determine if
> a node is out of recovery of not, and this makes particularly more
> sense since 9.6 where wal_level = archive <=> hot_standby. The thought
> behind that is also partially that people complain that replication is
> too hard to understand for people.
>

In particular on the topic of confusion it would help. And just in general
not have mostly unnecessary parameters is a win.


//Magnus

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

[Robert Haas]

On Thu, Dec 1, 2016 at 8:28 PM, Michael Paquier
 wrote:
> On Fri, Dec 2, 2016 at 10:10 AM, Robert Haas  wrote:
>> On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs  wrote:
>>> * pg_basebackup -R
>>> will write recovery.trigger to data directory
>>> insert parameters postgresql.conf.auto, if possible
>>
>> Don't understand that last line; otherwise, +1.
>
> pg_basebackup -R creates a recovery.conf now, by appending the
> parameters to postgresql.conf.auto we are sure that:
> 1) there is no need to check for the existence of recovery.conf as it
> could be included by postgresql.conf with something like an
> include_if_exists
> 2) postgresql.conf.auto is loaded automatically without any additional
> tweaks needed in the GUC parsing code paths.

Well, as to #1, we're making that an error IIUC.  But I see the point
of #2, for sure.  So sounds good to me.

>>> * Add docs: "Guide to changes in recovery.conf in 10.0"
>>
>> Hmm, we don't usually write the docs in terms of how things are
>> different from a previous version.  Might seem strange in 5 years.
>> Not sure what's best, here.
>
> A good chunk in the release notes would make sense as well?

Or instead.

>>> * remove hot_standby parameter altogether, in line with earlier changes
>>
>> That seems a little surprising.  We don't think anyone ever wants to
>> refuse connections during archive recovery?
>
> I suggested that yesterday. We have talked as well about merging
> standby_mode with hot_standby, but at the end most use cases I have
> seen involve looking at pg_is_in_recovery() these days to determine if
> a node is out of recovery of not, and this makes particularly more
> sense since 9.6 where wal_level = archive <=> hot_standby. The thought
> behind that is also partially that people complain that replication is
> too hard to understand for people.

If it were up to me, I'd keep the hot_standby parameter around but
default it to 'on'.

>>> * trigger_file renamed to promote_trigger_file
>>
>> Why?
>
> Because this is a trigger file aimed at doing promotion, not something else.

Oh, I see.  Makes sense.  I was confused.

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


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

Re: [HACKERS] Logical Replication WIP

[Thomas Munro]

On Fri, Dec 2, 2016 at 2:32 PM, Peter Eisentraut
 wrote:
> On 11/30/16 8:06 PM, Petr Jelinek wrote:
>> On 30/11/16 22:37, Peter Eisentraut wrote:
>>> I have taken the libpqwalreceiver refactoring patch and split it into
>>> two: one for the latch change, one for the API change.  I have done some
>>> mild editing.
>>>
>>> These two patches are now ready to commit in my mind.
>
>> Hi, looks good to me, do you plan to commit this soon or would you
>> rather me to resubmit the patches rebased on top of this (and including
>> this) first?
>
> committed those two

Commit 597a87ccc9a6fa8af7f3cf280b1e24e41807d555 left some comments
behind that referred to the select() that it removed.  Maybe rewrite
like in the attached?

I wonder if it would be worth creating and reusing a WaitEventSet here.

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


comments.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] PSQL commands: \quit_if, \quit_unless

[Robert Haas]

On Tue, Nov 29, 2016 at 4:43 AM, Pavel Stehule  wrote:
> I prefer the commands instead symbols - the parsing and processing symbols
> should be more complex than it is now. A psql parser is very simple - and
> any complex syntax enforces lot of code.
>
> \if_not

Given the precedent in pgbench (cf.
878fdcb843e087cc1cdeadc987d6ef55202ddd04), I think it requires an
amazing level of optimism to suppose we won't eventually end up with a
full-blown expression language here.  I would suggest designing one
from the beginning and getting it over with.  Even if you manage to
hold the line and exclude it from whatever gets committed initially,
somebody's going to propose it 2 years from now.  And again 4 years
from now.  And again 2 years after that.

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


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

Re: [HACKERS] Broken SSL tests in master

[Peter Eisentraut]

On 12/1/16 4:53 PM, Michael Paquier wrote:
> The reason why the SSL test suite is not in check-world is that SSL
> cannot be used with unix domain sockets, making it unfit in shared
> environments.

If that is it, that could be changed.

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


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

Re: [HACKERS] XactLockTableWait doesn't set wait_event correctly

[Robert Haas]

On Thu, Dec 1, 2016 at 7:35 PM, Simon Riggs  wrote:
> On 2 December 2016 at 00:28, Robert Haas  wrote:
>> On Wed, Nov 30, 2016 at 6:50 AM, Simon Riggs  wrote:
>>> Obtaining a tuple lock requires two separate actions: First we do
>>> LockTuple() and then we do XactLockTableWait().
>>
>> I think that's kind of a confusing way of looking at it.  LockTuple()
>> waits for a "tuple" lmgr lock, and XactLockTableWait waits for a
>> "transaction" lmgr lock.  Those two things are both part of a larger
>> protocol for managing access to what we refer to as tuple locks at the
>> SQL level.  I don't think conflating those things would be a very good
>> idea, because it's useful to know which phase you're currently doing -
>> e.g. anybody waiting on a tuple lock is not first in the queue.
>
> Why is it useful to know which phase you're at? What can I do with that info?

Well, I just mentioned one thing you can do with it...

> Why is knowing that you're doing a "transaction lock" more important
> than the fact you're doing a tuple lock on a particular db, relation,
> page and tuple? The "transaction lock" tells you nothing a user can
> act upon to improve the situation.

I think the charter of pg_locks is to provide visibility into what's
actually going on in the lock table.  It could be useful to provide
additional supplementary information as well, if we can do that in a
reasonable way, but I think trying to paper over the fact that
internally it's a transaction lock probably isn't a good idea.  For
example, suppose next week somebody decides to replace the "waiting"
column in pg_locks with a column indicating the position in the lock
queue, with NULL indicating a granted lock.   Well, if transaction
locks look like tuple locks, the person writing that patch will find
it impossible to display something sane in those cases, because the
transaction lock queue is separate from the tuple lock queue and you
can't somehow glue those together without distorting what the lock
manager is actually doing.  That kind of thing is the natural
consequence of displaying something other than what the system is
actually doing internally.  But I have no objection to you adding
supplementary detail also.

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


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

Re: [HACKERS] Logical Replication WIP

[Peter Eisentraut]

On 11/30/16 8:06 PM, Petr Jelinek wrote:
> On 30/11/16 22:37, Peter Eisentraut wrote:
>> I have taken the libpqwalreceiver refactoring patch and split it into
>> two: one for the latch change, one for the API change.  I have done some
>> mild editing.
>>
>> These two patches are now ready to commit in my mind.

> Hi, looks good to me, do you plan to commit this soon or would you
> rather me to resubmit the patches rebased on top of this (and including
> this) first?

committed those two

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


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

Re: [HACKERS] Broken SSL tests in master

[Michael Paquier]

On Fri, Dec 2, 2016 at 10:26 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Would it be better to return NULL instead then.
>
> That would likely just result in application core dumps.
> See notes for commit 490cb21f7.

That's 40cb21f7 actually. Thanks for the pointer.

> I think we've established an expectation
> that only a NULL argument will elicit a NULL return from PQhost.

OK, the current behavior wins then.
-- 
Michael


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

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

[Michael Paquier]

On Fri, Dec 2, 2016 at 10:10 AM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs  wrote:
>> * pg_basebackup -R
>> will write recovery.trigger to data directory
>> insert parameters postgresql.conf.auto, if possible
>
> Don't understand that last line; otherwise, +1.

pg_basebackup -R creates a recovery.conf now, by appending the
parameters to postgresql.conf.auto we are sure that:
1) there is no need to check for the existence of recovery.conf as it
could be included by postgresql.conf with something like an
include_if_exists
2) postgresql.conf.auto is loaded automatically without any additional
tweaks needed in the GUC parsing code paths.

>> * Add docs: "Guide to changes in recovery.conf in 10.0"
>
> Hmm, we don't usually write the docs in terms of how things are
> different from a previous version.  Might seem strange in 5 years.
> Not sure what's best, here.

A good chunk in the release notes would make sense as well?

>> * recovery_target as a single parameter, using proposed "xid 666"
>> "target value" format
>
> +1.
>
>> * remove hot_standby parameter altogether, in line with earlier changes
>
> That seems a little surprising.  We don't think anyone ever wants to
> refuse connections during archive recovery?

I suggested that yesterday. We have talked as well about merging
standby_mode with hot_standby, but at the end most use cases I have
seen involve looking at pg_is_in_recovery() these days to determine if
a node is out of recovery of not, and this makes particularly more
sense since 9.6 where wal_level = archive <=> hot_standby. The thought
behind that is also partially that people complain that replication is
too hard to understand for people.

>> * trigger_file renamed to promote_trigger_file
>
> Why?

Because this is a trigger file aimed at doing promotion, not something else.
-- 
Michael


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

Re: [HACKERS] Broken SSL tests in master

[Tom Lane]

Michael Paquier  writes:
> On Fri, Dec 2, 2016 at 8:36 AM, Robert Haas  wrote:
>> Correct, but I'm defining that as user error.  If hostaddr is
>> specified, host is not used to decide what to connect to, so it makes
>> no sense for it to be a string of multiple host names.  If we allowed
>> multiple hostaddrs, as has been proposed, then we'd need to be more
>> clever about this.

> Would it be better to return NULL instead then.

That would likely just result in application core dumps.
See notes for commit 490cb21f7.  I think we've established an expectation
that only a NULL argument will elicit a NULL return from PQhost.

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Michael Paquier]

On Fri, Dec 2, 2016 at 8:36 AM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 4:42 PM, Michael Paquier
>  wrote:
>> On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas  wrote:
>>> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
 As you can see, after the patch libpq will now look at hostaddr rather than
 host when validating the server certificate because that is what is stored
 in the first (and only) entry of conn->connhost, and therefore what 
 PQhost()
 return.

 To me it feels like the proper fix would be to make PQHost() return the
 value of the host parameter rather than the hostaddr (maybe add a new field
 in the pg_conn_host struct). But would be a behaviour change which might
 break someones application. Thoughts?
>>>
>>> I think that the blame here is on the original commit,
>>> 274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
>>> the behavior of PQhost.  Prior to that commit, even if "hostaddr" was
>>> used, PQhost would still return whatever value was associated with the
>>> "host" parameter, but now it ignores "host" and returns "hostaddr"
>>> instead.  That's busted.  I've pushed a trivial fix, and the SSL tests
>>> now pass for me.
>>
>> +   if (conn->connhost != NULL &&
>> +   conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
>> return conn->connhost[conn->whichhost].host
>> I think that's still incorrect. If a connection string defines a
>> comma-separated list of host, and hostaddr is defined as well,
>> PQhost() would return the comma-separated list, not the IP of the host
>> it is connected to. Am I reading that incorrectly?
>
> Correct, but I'm defining that as user error.  If hostaddr is
> specified, host is not used to decide what to connect to, so it makes
> no sense for it to be a string of multiple host names.  If we allowed
> multiple hostaddrs, as has been proposed, then we'd need to be more
> clever about this.

Would it be better to return NULL instead then. Falling back to a
comma-separated list of hosts, or the default values does not sound
much appealing either.
-- 
Michael


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

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

[Robert Haas]

On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs  wrote:
> * Move recovery.conf parameters into postgresql.conf
> Allow reload of most parameters, allow ALTER SYSTEM
> Provide visibility of values through GUC interface

+1.

> * recovery.conf is replaced by   recovery.trigger -> recovery.done

+1.

> * pg_basebackup -R
> will write recovery.trigger to data directory
> insert parameters postgresql.conf.auto, if possible

Don't understand that last line; otherwise, +1.

> * backwards compatibility - read recovery.conf from $DATADIR -
> presence of recovery.conf will cause ERROR

+1.

> * backwards compatibility - some parameter names will change, so
> allows others to changes also if needed

Don't understand this.

> * Add docs: "Guide to changes in recovery.conf in 10.0"

Hmm, we don't usually write the docs in terms of how things are
different from a previous version.  Might seem strange in 5 years.
Not sure what's best, here.

> * recovery_target as a single parameter, using proposed "xid 666"
> "target value" format

+1.

> * remove hot_standby parameter altogether, in line with earlier changes

That seems a little surprising.  We don't think anyone ever wants to
refuse connections during archive recovery?

> * trigger_file renamed to promote_trigger_file

Why?

> * standby_mode = on| off -> default to on

+1.

> * pg_ctl recover - not as part of this patch

+1.

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


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

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch 

> I've not applied patch patch_pg_current_logfile-v14.diff.backoff to
> prevent constant call of logfile_writename() on a busy system (errno =
> ENFILE | EMFILE).

I don't think it should be applied and included in the basic
functionality patch in any case. I think it needs to be submitted as a
separate patch along with the basic functionality patch.  Backing off
the retry of the current_logfiles write could be overly fancy and
simply not needed.

> I think this can be done quite simply by testing if
> log rotate is still enabled. This is possible because function
> logfile_rotate() is already testing if  errno = ENFILE | EMFILE and in
> this case rotation_disabled is set to true. So the following test
> should do the work:
> 
>if (log_metainfo_stale && !rotation_disabled)
>logfile_writename();
> 
> This is included in v15 patch.

I don't see this helping much, if at all.

First, it's not clear just when rotation_disabled can be false
when log_metainfo_stale is true.  The typical execution
path is for logfile_writename() to be called after rotate_logfile()
has already set rotataion_disabled to true.   logfile_writename()
is the only place setting log_metainfo_stale to true and
rotate_logfile() the only place settig rotation_disabled to false.

While it's possible
that log_metainfo_stale might be set to true when logfile_writename()
is called from within open_csvlogfile(), this does not help with the
stderr case.  IMO better to just test for log_metaifo_stale at the
code snippet above.

Second, the whole point of retrying the logfile_writename() call is
to be sure that the current_logfiles file is updated before the logs
rotate.  Waiting until logfile rotation is enabled defeats the purpose.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] XactLockTableWait doesn't set wait_event correctly

[Simon Riggs]

On 2 December 2016 at 00:28, Robert Haas  wrote:
> On Wed, Nov 30, 2016 at 6:50 AM, Simon Riggs  wrote:
>> Obtaining a tuple lock requires two separate actions: First we do
>> LockTuple() and then we do XactLockTableWait().
>
> I think that's kind of a confusing way of looking at it.  LockTuple()
> waits for a "tuple" lmgr lock, and XactLockTableWait waits for a
> "transaction" lmgr lock.  Those two things are both part of a larger
> protocol for managing access to what we refer to as tuple locks at the
> SQL level.  I don't think conflating those things would be a very good
> idea, because it's useful to know which phase you're currently doing -
> e.g. anybody waiting on a tuple lock is not first in the queue.

Why is it useful to know which phase you're at? What can I do with that info?

Why is knowing that you're doing a "transaction lock" more important
than the fact you're doing a tuple lock on a particular db, relation,
page and tuple? The "transaction lock" tells you nothing a user can
act upon to improve the situation.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

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

[Simon Riggs]

On 29 November 2016 at 15:13, Simon Riggs  wrote:
> On 14 November 2016 at 15:50, Robert Haas  wrote:
>> On Sat, Nov 12, 2016 at 11:09 AM, Andres Freund  wrote:
>>> I'm very tempted to rename this during the move to GUCs
>> ...
>>> Slightly less so, but still tempted to also rename these. They're not
>>> actually necessarily pointing towards a primary, and namespace-wise
>>> they're not grouped with recovery_*, which has become more important now
>>> that recovery.conf isn't a separate namespace anymore.
>>
>> -1 for renaming these.  I don't think the current names are
>> particularly bad, and I think trying to agree on what would be better
>> could easily sink the whole patch.
>
> OK, so we can move forward. Thanks.
>
> I'm going to be doing final review and commit this week, at the
> Developer meeting on Thurs and on Friday, with input in person and on
> list.

err... no I'm not, based on review feedback in Tokyo.

New schedule is roughly this...
* agree changes over next week
* make changes and submit new patch by 1 Jan
* commit patch by 7 Jan

Overview of details agreed in Tokyo, now subject to further comments
from hackers

* Move recovery.conf parameters into postgresql.conf
Allow reload of most parameters, allow ALTER SYSTEM
Provide visibility of values through GUC interface

* recovery.conf is replaced by   recovery.trigger -> recovery.done

* pg_basebackup -R
will write recovery.trigger to data directory
insert parameters postgresql.conf.auto, if possible

* backwards compatibility - read recovery.conf from $DATADIR -
presence of recovery.conf will cause ERROR
* backwards compatibility - some parameter names will change, so
allows others to changes also if needed

* Add docs: "Guide to changes in recovery.conf in 10.0"

* recovery_target as a single parameter, using proposed "xid 666"
"target value" format

* remove hot_standby parameter altogether, in line with earlier changes

* trigger_file renamed to promote_trigger_file

* standby_mode = on| off -> default to on

* pg_ctl recover - not as part of this patch

I'll work on the patch from here on, to allow me to work towards commit.

Comments please.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] XactLockTableWait doesn't set wait_event correctly

[Robert Haas]

On Wed, Nov 30, 2016 at 6:50 AM, Simon Riggs  wrote:
> Obtaining a tuple lock requires two separate actions: First we do
> LockTuple() and then we do XactLockTableWait().

I think that's kind of a confusing way of looking at it.  LockTuple()
waits for a "tuple" lmgr lock, and XactLockTableWait waits for a
"transaction" lmgr lock.  Those two things are both part of a larger
protocol for managing access to what we refer to as tuple locks at the
SQL level.  I don't think conflating those things would be a very good
idea, because it's useful to know which phase you're currently doing -
e.g. anybody waiting on a tuple lock is not first in the queue.

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


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

Re: [HACKERS] Mail thread references in commits

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Andres Freund  writes:
> > This:
> 
> >> Discussion: 
> >> https://postgr.es/m/20161128182113.6527.58...@wrigleys.postgresql.org
> >> Discussion: 
> >> https://postgr.es/m/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com
> 
> > still looks better than:
> 
> >> Discussion: 
> >> http://www.postgresql.org/message-id/20161128182113.6527.58...@wrigleys.postgresql.org
> >> Discussion: 
> >> http://www.postgresql.org/message-id/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com
> 
> Really?  Somebody expressed the opposite preference upthread, and I agree.
> "postgr.es" looks, at best, unofficial.

I find that to be a particularly compelling point.

Also, while it's a pretty minor point, using the canonical form also
avoids an unnecessary redirect.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Write Ahead Logging for Hash Indexes

[Amit Kapila]

On Thu, Dec 1, 2016 at 9:44 PM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 1:03 AM, Amit Kapila  wrote:
>> On Wed, Nov 9, 2016 at 7:40 PM, Amit Kapila  wrote:
>>> On Tue, Nov 8, 2016 at 10:56 PM, Jeff Janes  wrote:
 Unless we want to wait until that work is committed before doing more 
 review
 and testing on this.
>>>
>>> The concurrent hash index patch is getting changed and some of the
>>> changes needs change in this patch as well.  So, I think after it gets
>>> somewhat stabilized, I will update this patch as well.
>>
>> Now that concurrent hash index patch is committed [1], I will work on
>> rebasing this patch.  Note, I have moved this to next CF.
>
> Thanks.  I am thinking that it might make sense to try to get the
> "microvacuum support for hash index" and "cache hash index meta page"
> patches committed before this one, because I'm guessing they are much
> simpler than this one, and I think therefore that the review of those
> patches can probably move fairly quickly.
>

I think it makes sense to move "cache hash index meta page" first,
however "microvacuum support for hash index" is based on WAL patch as
the action in this patch (delete op) also needs to be logged.  One
idea could be that we can try to split the patch so that WAL logging
can be done as a separate patch, but I am not sure if it is worth.

>  Of course, ideally I can
> also start reviewing this one in the meantime.  Does that make sense
> to you?
>

You can start reviewing some of the operations like "Create Index",
"Insert".  However, some changes are required because of change in
locking strategy for Vacuum.  I am planning to work on rebasing it and
making required changes in next week.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Thu, Dec 1, 2016 at 4:42 PM, Michael Paquier
 wrote:
> On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas  wrote:
>> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
>>> As you can see, after the patch libpq will now look at hostaddr rather than
>>> host when validating the server certificate because that is what is stored
>>> in the first (and only) entry of conn->connhost, and therefore what PQhost()
>>> return.
>>>
>>> To me it feels like the proper fix would be to make PQHost() return the
>>> value of the host parameter rather than the hostaddr (maybe add a new field
>>> in the pg_conn_host struct). But would be a behaviour change which might
>>> break someones application. Thoughts?
>>
>> I think that the blame here is on the original commit,
>> 274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
>> the behavior of PQhost.  Prior to that commit, even if "hostaddr" was
>> used, PQhost would still return whatever value was associated with the
>> "host" parameter, but now it ignores "host" and returns "hostaddr"
>> instead.  That's busted.  I've pushed a trivial fix, and the SSL tests
>> now pass for me.
>
> +   if (conn->connhost != NULL &&
> +   conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
> return conn->connhost[conn->whichhost].host
> I think that's still incorrect. If a connection string defines a
> comma-separated list of host, and hostaddr is defined as well,
> PQhost() would return the comma-separated list, not the IP of the host
> it is connected to. Am I reading that incorrectly?

Correct, but I'm defining that as user error.  If hostaddr is
specified, host is not used to decide what to connect to, so it makes
no sense for it to be a string of multiple host names.  If we allowed
multiple hostaddrs, as has been proposed, then we'd need to be more
clever about this.

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


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

Re: [HACKERS] Wrong order of tests in findDependentObjects()

[Tom Lane]

Jim Nasby  writes:
> On 12/1/16 1:09 PM, Tom Lane wrote:
>> I think that the patch I wrote is good cleanup, so I'm still inclined
>> to apply it in HEAD, but I no longer think it's fixing any case that's
>> significant in the field.  I wonder if you have a counterexample?

> No; I'm sure I've run into this because of a temp object other than a 
> table (probably a function, though possibly something else).

Makes sense.  The fact that we protect against this for temp tables and
views would make it all the more surprising when you get bit by some
less-common object type.

regards, tom lane


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

Re: [HACKERS] Mail thread references in commits

[Tom Lane]

Andres Freund  writes:
> This:

>> Discussion: 
>> https://postgr.es/m/20161128182113.6527.58...@wrigleys.postgresql.org
>> Discussion: 
>> https://postgr.es/m/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com

> still looks better than:

>> Discussion: 
>> http://www.postgresql.org/message-id/20161128182113.6527.58...@wrigleys.postgresql.org
>> Discussion: 
>> http://www.postgresql.org/message-id/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com

Really?  Somebody expressed the opposite preference upthread, and I agree.
"postgr.es" looks, at best, unofficial.

regards, tom lane


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

Re: [HACKERS] Mail thread references in commits

[Andres Freund]

On 2016-12-01 18:12:34 -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Dec 1, 2016 at 4:33 PM, Tom Lane  wrote:
> >> When and if somebody tries to game that, we can do something about it,
> >> but I'm not very worried.  It's not like it's not trivial to get your
> >> company's name, or $badword of your choice, into the archives already.
> 
> > Sure, of course.  But it's a bit different when it's in the commit log.
> 
> Sure, but there's a filter in that case: if a committer finds a messageID
> sufficiently offensive, he can just not quote it in the commit message.
> There's precedent for that; I've more than once omitted crediting
> somebody's bug report when it was submitted under an obviously fake name.
> 
> I'm not finding myself fussed about this.

+1. I mean if people behave intentionally offensive, we can always tell
them and starting ignoring them. Seems like they'd stop doing that soon,
because their patches won't get in/their bugs won't get fixed.


-- 
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] Wrong order of tests in findDependentObjects()

[Jim Nasby]

On 12/1/16 1:09 PM, Tom Lane wrote:

I think that the patch I wrote is good cleanup, so I'm still inclined
to apply it in HEAD, but I no longer think it's fixing any case that's
significant in the field.  I wonder if you have a counterexample?


No; I'm sure I've run into this because of a temp object other than a 
table (probably a function, though possibly something else).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Mail thread references in commits

[Tom Lane]

Robert Haas  writes:
> On Thu, Dec 1, 2016 at 4:33 PM, Tom Lane  wrote:
>> When and if somebody tries to game that, we can do something about it,
>> but I'm not very worried.  It's not like it's not trivial to get your
>> company's name, or $badword of your choice, into the archives already.

> Sure, of course.  But it's a bit different when it's in the commit log.

Sure, but there's a filter in that case: if a committer finds a messageID
sufficiently offensive, he can just not quote it in the commit message.
There's precedent for that; I've more than once omitted crediting
somebody's bug report when it was submitted under an obviously fake name.

I'm not finding myself fussed about this.

regards, tom lane


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

Re: [HACKERS] Mail thread references in commits

[Andres Freund]

On 2016-12-01 18:05:19 -0500, Tom Lane wrote:
> ... the shortener isn't really doing anything for us.  You end up with a
> line longer than 80 characters with message-IDs generated by either gmail
> or the bug report form, for instance these examples from recent commits:

Still seems quite useful to me. I don't really see much argument
against.

This:

> Discussion: 
> https://postgr.es/m/20161128182113.6527.58...@wrigleys.postgresql.org
> Discussion: 
> https://postgr.es/m/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com

still looks better than:

> Discussion: 
> http://www.postgresql.org/message-id/20161128182113.6527.58...@wrigleys.postgresql.org
> Discussion: 
> http://www.postgresql.org/message-id/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com

and I see little downside. If we can't keep the first stable, we can't
keep the second one stable either.

> Another convention that apparently needs to be discussed is whether or
> not to point at a whole thread, as in this example from yesterday:
>
> Discussion: 
> http://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F5EE995@G01JPEXMBYT05/
>
> I'm kind of -1 on that; it's adding 5 characters for not much.

-1 on that too.

Andres


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

Re: [HACKERS] Mail thread references in commits

[Tom Lane]

Stephen Frost  writes:
> Further, we seem agreed that URLs are what we want to have in the
> commits rather than just the message-ID.

If we're set on doing that, then ...

> The question on the table at the moment seems to be if we want to use
> https://postgr.es/m/ or https://postgresql.org/message-id/ as the
> prefix.  Personally, I don't really care and would prefer we just decide
> something and move on to more interesting technical discussion.  I don't
> really agree with the complaints levied against https://postgr.es/m/,
> but I'm also not particularly bothered by one long line in each commit
> message.

... the shortener isn't really doing anything for us.  You end up with a
line longer than 80 characters with message-IDs generated by either gmail
or the bug report form, for instance these examples from recent commits:

Discussion: 
https://postgr.es/m/20161128182113.6527.58...@wrigleys.postgresql.org
Discussion: 
https://postgr.es/m/CA+renyUEE29=X01JXdz8_TQvo6n9=2xoebbrnq8rklyr+kj...@mail.gmail.com

And those two sources comprise the majority of references these days.
Even dropping the Discussion: keyword wouldn't get us to lines that don't
wrap.  So we might as well go with the canonical form, which I take to
be https://www.postgresql.org/message-id/.

Another convention that apparently needs to be discussed is whether or
not to point at a whole thread, as in this example from yesterday:

Discussion: 
http://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F5EE995@G01JPEXMBYT05/

I'm kind of -1 on that; it's adding 5 characters for not much.

regards, tom lane


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

Re: [HACKERS] Mail thread references in commits

[Stephen Frost]

All,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> I think this is a straw man.  We've already decided to use message-IDs
> as the basic identity of messages for this purpose; other proposals
> were considered before and rejected as too inconvenient.

I tend to agree with Tom on this, for better or worse, message-ID is
what we're using.

Further, we seem agreed that URLs are what we want to have in the
commits rather than just the message-ID.

The question on the table at the moment seems to be if we want to use
https://postgr.es/m/ or https://postgresql.org/message-id/ as the
prefix.  Personally, I don't really care and would prefer we just decide
something and move on to more interesting technical discussion.  I don't
really agree with the complaints levied against https://postgr.es/m/,
but I'm also not particularly bothered by one long line in each commit
message.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Mail thread references in commits

[Robert Haas]

On Thu, Dec 1, 2016 at 4:33 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Nov 30, 2016 at 1:50 PM, Greg Stark  wrote:
>>> I can't say I feel especially strongly either way on this but just to
>>> toss out a small thing that might make a small difference
>>>
>>> If you happen to know how your message-ids are generated then you
>>> might be able to do something useful with them. For instance, you
>>> could search all git commits for urls to messages you wrote -- for
>>> instance any commit that has CAB7nPq is referencing a message written
>>> by Michael Paquier.
>>>
>>> On the other hand you could put something naughty in the message-id
>>> forcing everyone else to use URLs with dirty words in them. Or with
>>> words like "terrorist" in them. Or with some javascript/html injection
>>> attack of some sort...
>
>> ...or the name of your company/your email hosting provider's company...
>
> I think this is a straw man.  We've already decided to use message-IDs
> as the basic identity of messages for this purpose; other proposals
> were considered before and rejected as too inconvenient.
>
> When and if somebody tries to game that, we can do something about it,
> but I'm not very worried.  It's not like it's not trivial to get your
> company's name, or $badword of your choice, into the archives already.
> The former is more or less standard practice, in fact, as per this
> very message:

Sure, of course.  But it's a bit different when it's in the commit log.

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


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Tom Lane]

I wrote:
> Yeah, I didn't have any doubt that it was real.  Still don't know
> why my test case isn't doing what I expected, though.

Doh: the planner knows that transaction_timestamp() is stable, so
it concludes that the DISTINCT condition is vacuous.  There is a
"Unique" node in the plan, but it has zero columns to compare, so
it thinks the tuple are all equivalent and emits only the first.

I had noticed that there was no "Sort" node, but failed to realize
that that implied the "Unique" node was degenerate.

Maybe this is over-optimization, but I think we'd be very sad if
the planner didn't do it; getting rid of useless sort columns is
critical in a lot of situations.

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Michael Paquier]

On Fri, Dec 2, 2016 at 5:46 AM, Robert Haas  wrote:
> On Thu, Dec 1, 2016 at 3:40 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane  wrote:
 check-world isn't a magic bullet.
>>
>>> No, but deliberately leaving things out that could be run isn't
>>> helping anything either.
>>
>> Tests that open security holes while running aren't in my list of "things
>> that could be run automatically".
>
> If we create a new target that runs them automatically, you don't have
> to use it.

Having a new target would make sense, if flagged as security-unsafe.
The reason why the SSL test suite is not in check-world is that SSL
cannot be used with unix domain sockets, making it unfit in shared
environments.
-- 
Michael


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

Re: [HACKERS] Broken SSL tests in master

[Michael Paquier]

On Fri, Dec 2, 2016 at 5:56 AM, Robert Haas  wrote:
> On Fri, Nov 25, 2016 at 4:16 AM, Mithun Cy  wrote:
>> Reason is we first decode the URI(percent encoded character) then try to
>> split the string into multiple host assuming they are separated by ','. I
>> think we need to change the order here. Otherwise difficult the say whether
>> ',' is part of USD path or a separator.
>
> Yeah, we should change that.  Are you going to write a patch?

The interesting bit in rfc3986:

   Aside from dot-segments in hierarchical paths, a path segment is
   considered opaque by the generic syntax.  URI producing applications
   often use the reserved characters allowed in a segment to delimit
   scheme-specific or dereference-handler-specific subcomponents.  For
   example, the semicolon (";") and equals ("=") reserved characters are
   often used to delimit parameters and parameter values applicable to
   that segment.  The comma (",") reserved character is often used for
   similar purposes.  For example, one URI producer might use a segment
   such as "name;v=1.1" to indicate a reference to version 1.1 of
   "name", whereas another might use a segment such as "name,1.1" to
   indicate the same.  Parameter types may be defined by scheme-specific
   semantics, but in most cases the syntax of a parameter is specific to
   the implementation of the URI's dereferencing algorithm.

So not being able to distinguish commas in names and separators is
clearly a bug.
-- 
Michael


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

Re: [HACKERS] Broken SSL tests in master

[Michael Paquier]

On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas  wrote:
> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
>> As you can see, after the patch libpq will now look at hostaddr rather than
>> host when validating the server certificate because that is what is stored
>> in the first (and only) entry of conn->connhost, and therefore what PQhost()
>> return.
>>
>> To me it feels like the proper fix would be to make PQHost() return the
>> value of the host parameter rather than the hostaddr (maybe add a new field
>> in the pg_conn_host struct). But would be a behaviour change which might
>> break someones application. Thoughts?
>
> I think that the blame here is on the original commit,
> 274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
> the behavior of PQhost.  Prior to that commit, even if "hostaddr" was
> used, PQhost would still return whatever value was associated with the
> "host" parameter, but now it ignores "host" and returns "hostaddr"
> instead.  That's busted.  I've pushed a trivial fix, and the SSL tests
> now pass for me.

+   if (conn->connhost != NULL &&
+   conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
return conn->connhost[conn->whichhost].host
I think that's still incorrect. If a connection string defines a
comma-separated list of host, and hostaddr is defined as well,
PQhost() would return the comma-separated list, not the IP of the host
it is connected to. Am I reading that incorrectly?
-- 
Michael


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

Re: [HACKERS] Proposal: scan key push down to heap [WIP]

[Andres Freund]

On 2016-11-30 16:11:23 +0530, Dilip Kumar wrote:
> On Tue, Nov 29, 2016 at 11:21 PM, Robert Haas  wrote:
> > On Mon, Nov 28, 2016 at 11:17 PM, Dilip Kumar  wrote:
> >> Actually we want to call slot_getattr instead heap_getattr, because of
> >> problem mentioned by Andres upthread and we also saw in test results.
> >
> > Ah, right.
> >
> >> Should we make a copy of HeapKeyTest lets say ExecKeyTest and keep it
> >> under executor ?
> >
> > Sure.
> 
> I have worked on the idea you suggested upthread. POC patch is
> attached.

Hm. I'm more than a bit doubful about this approach. Shouldn't we just
*always* do this as part of expression evaluation, instead of
special-casing for seqscans?

I.e. during planning recognize that an OpExpr can be evaluated as a
scankey and then emit different qual evaluation instructions?  Because
then the benefit can be everywhere, instead of just seqscans.

I'll post my new expression evaluation stuff - which doesn't do this
atm, but makes ExecQual faster in other ways - later this week.  If we
could get the planner (or parse-analysis?) to set an OpExpr flag that
signals that the expression can be evaluated as a scankey, that'd be
easy.

Greetings,

Andres Freund


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

Re: [HACKERS] Mail thread references in commits

[Tom Lane]

Robert Haas  writes:
> On Wed, Nov 30, 2016 at 1:50 PM, Greg Stark  wrote:
>> I can't say I feel especially strongly either way on this but just to
>> toss out a small thing that might make a small difference
>> 
>> If you happen to know how your message-ids are generated then you
>> might be able to do something useful with them. For instance, you
>> could search all git commits for urls to messages you wrote -- for
>> instance any commit that has CAB7nPq is referencing a message written
>> by Michael Paquier.
>> 
>> On the other hand you could put something naughty in the message-id
>> forcing everyone else to use URLs with dirty words in them. Or with
>> words like "terrorist" in them. Or with some javascript/html injection
>> attack of some sort...

> ...or the name of your company/your email hosting provider's company...

I think this is a straw man.  We've already decided to use message-IDs
as the basic identity of messages for this purpose; other proposals
were considered before and rejected as too inconvenient.

When and if somebody tries to game that, we can do something about it,
but I'm not very worried.  It's not like it's not trivial to get your
company's name, or $badword of your choice, into the archives already.
The former is more or less standard practice, in fact, as per this
very message:

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

regards, tom lane


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Tom Lane]

Robert Haas  writes:
> On Thu, Dec 1, 2016 at 3:46 PM, Tom Lane  wrote:
>> but it doesn't:
>> 
>> regression=# select distinct transaction_timestamp() from tenk1;
>> transaction_timestamp
>> ---
>> 2016-12-01 15:44:12.839417-05
>> (1 row)
>> 
>> How is that happening?

> Because the table is so small, the leader probably finishes running
> the whole plan before the workers finish starting up.

Good try, but EXPLAIN ANALYZE says that the workers are processing
some of the rows.  Also, I see the same behavior with a much larger
test table.

> You can see the problem like this, though:

Yeah, I didn't have any doubt that it was real.  Still don't know
why my test case isn't doing what I expected, though.

regards, tom lane


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

Re: [HACKERS] Wrong order of tests in findDependentObjects()

[Tom Lane]

I wrote:
>> Jim Nasby  writes:
>>> I can't think of any reason you'd want the current behavior.

>> But I think fixing it to not recurse to extensions during temp namespace
>> cleanup might not be very hard.  I'll take a look.

I wrote a test case to try to demonstrate that this patch was fixing a
bug, and was surprised to find that it didn't fail.  The reason turns
out to be that we fixed this problem years ago in commit 08dd23cec:

Also, arrange for explicitly temporary tables to not get linked as
extension members in the first place, and the same for the magic
pg_temp_nnn schemas that are created to hold them.  This prevents assorted
unpleasant results if an extension script creates a temp table: the forced
drop at session end would either fail or remove the entire extension, and
neither of those outcomes is desirable.

Now, if you really try hard, say by creating a temp function, you can
break it.  But I don't have all that much sympathy for such use-cases.

I think that the patch I wrote is good cleanup, so I'm still inclined
to apply it in HEAD, but I no longer think it's fixing any case that's
significant in the field.  I wonder if you have a counterexample?

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Fri, Nov 25, 2016 at 4:16 AM, Mithun Cy  wrote:
> On Fri, Nov 25, 2016 at 12:03 PM, Andreas Karlsson 
> wrote:
>> Another thought about this code: should we not check if it is a unix
>> socket first before splitting the host? While I doubt that it is common to
>> have a unix >socket in a directory with comma in the path it is a bit
>> annoying that we no longer support this.
>
> I think it is a bug.
>
> Before this feature:
>
> ./psql postgres://%2fhome%2fmithun%2f%2c
> psql: could not connect to server: No such file or directory
> Is the server running locally and accepting
> connections on Unix domain socket "/home/mithun/,/.s.PGSQL.5444"?
>
> After this feature:
> ./psql postgres://%2fhome%2fmithun%2f%2c
> psql: could not connect to server: No such file or directory
> Is the server running locally and accepting
> connections on Unix domain socket "/home/mithun//.s.PGSQL.5432"?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
>
> So comma (%2c) is misinterpreted as separator not as part of UDS path.
>
> Reason is we first decode the URI(percent encoded character) then try to
> split the string into multiple host assuming they are separated by ','. I
> think we need to change the order here. Otherwise difficult the say whether
> ',' is part of USD path or a separator.

Yeah, we should change that.  Are you going to write a patch?

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


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

Re: [HACKERS] Mail thread references in commits

[Robert Haas]

On Wed, Nov 30, 2016 at 1:50 PM, Greg Stark  wrote:
> On 30 November 2016 at 16:19, Andrew Dunstan  wrote:
>>
>> https://www.postgresql.org/message-id/cab7npqthydyf-fo+fzvxrhz-7_hptm4rodbcsy9-noqhvet...@mail.gmail.com
>>
>> I'll be interested to know if it breaks anyone's MUA. If it doesn't all we
>> will be arguing about are aesthetics, and I'm a firm believer in function
>> over form.
>
> I can't say I feel especially strongly either way on this but just to
> toss out a small thing that might make a small difference
>
> If you happen to know how your message-ids are generated then you
> might be able to do something useful with them. For instance, you
> could search all git commits for urls to messages you wrote -- for
> instance any commit that has CAB7nPq is referencing a message written
> by Michael Paquier.
>
> On the other hand you could put something naughty in the message-id
> forcing everyone else to use URLs with dirty words in them. Or with
> words like "terrorist" in them. Or with some javascript/html injection
> attack of some sort...

...or the name of your company/your email hosting provider's company...

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


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Robert Haas]

On Thu, Dec 1, 2016 at 3:46 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Dec 1, 2016 at 2:32 PM, Tom Lane  wrote:
>>> ... well, they would be if we passed down xactStartTimestamp to parallel
>>> workers, but I can't find any code that does that.  In view of the fact that
>>> transaction_timestamp() is marked as parallel-safe, this is a bug in 9.6.
>
>> Yeah.  Do you think we should arrange to pass that down, or change the 
>> marking?
>
> We can't fix the marking in existing 9.6 installations, so I think we
> have to pass it down.  (Which would be a better response anyway.)
>
> Having said that, I find myself unable to reproduce a problem.
> This should fail:
>
> regression=# set parallel_setup_cost TO 0;
> SET
> regression=# set parallel_tuple_cost TO 0;
> SET
> regression=# set min_parallel_relation_size TO 0;
> SET
> regression=# set enable_indexscan TO 0;
> SET
> regression=# explain verbose select distinct transaction_timestamp() from 
> tenk1;
>   QUERY PLAN
> --
>  Unique  (cost=0.00..424.67 rows=1 width=8)
>Output: (transaction_timestamp())
>->  Gather  (cost=0.00..424.67 rows=1 width=8)
>  Output: (transaction_timestamp())
>  Workers Planned: 2
>  ->  Parallel Seq Scan on public.tenk1  (cost=0.00..410.08 rows=4167 
> width=8)
>Output: transaction_timestamp()
> (7 rows)
>
> but it doesn't:
>
> regression=# select distinct transaction_timestamp() from tenk1;
>  transaction_timestamp
> ---
>  2016-12-01 15:44:12.839417-05
> (1 row)
>
> How is that happening?

Because the table is so small, the leader probably finishes running
the whole plan before the workers finish starting up.

You can see the problem like this, though:

rhaas=# begin;
BEGIN
rhaas=# select transaction_timestamp();
 transaction_timestamp
---
 2016-12-01 15:51:14.443116-05
(1 row)

rhaas=# select transaction_timestamp();
 transaction_timestamp
---
 2016-12-01 15:51:14.443116-05
(1 row)

rhaas=# select transaction_timestamp();
 transaction_timestamp
---
 2016-12-01 15:51:14.443116-05
(1 row)

rhaas=# set force_parallel_mode = true;
SET
rhaas=# select transaction_timestamp();
 transaction_timestamp
---
 2016-12-01 15:51:26.603302-05
(1 row)

rhaas=# select transaction_timestamp();
 transaction_timestamp
---
 2016-12-01 15:51:27.316032-05
(1 row)

force_parallel_mode causes the whole plan to be run by the worker,
without any participation by the leader.

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


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

Re: [HACKERS] Proposal: scan key push down to heap [WIP]

[Robert Haas]

On Wed, Nov 30, 2016 at 5:41 AM, Dilip Kumar  wrote:
> 1. As we decided to separate scankey and qual during planning time. So
> I am doing it at create_seqscan_path. My question is currently we
> don't have path node for seqscan, so where should we store scankey ?
> In Path node, or create new SeqScanPath node ?. In attached patch I
> have stored in Path node.

Well, Path should obviously only contain those things that are common
across all kinds of paths, so I would think you'd need to invent
SeqScanPath.

> 2. This is regarding instrumentation information for scan key, after
> my changes currently explain analyze result will look like this.
>
> postgres=# explain (analyze, costs off) select * from lineitem
> where l_shipmode in ('MAIL', 'AIR')
> and l_receiptdate >= date '1994-01-01';
> QUERY PLAN
> --
>  Seq Scan on lineitem (actual time=0.022..12179.946 rows=6238212 loops=1)
>Scan Key: (l_receiptdate >= '1994-01-01'::date)
>Filter: (l_shipmode = ANY ('{MAIL,AIR}'::bpchar[]))
>Rows Removed by Scan Key: 8162088
>Rows Removed by Filter: 15599495
>  Planning time: 0.182 ms
>  Execution time: 12410.529 ms
>
> My question is, how should we show pushdown filters  ?
> In above plan I am showing as  "Scan Key: ", does this look fine or we
> should name it something else ?

Seems OK to me, but I'm open to better ideas if anybody has any.

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


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Tom Lane]

Robert Haas  writes:
> On Thu, Dec 1, 2016 at 2:32 PM, Tom Lane  wrote:
>> ... well, they would be if we passed down xactStartTimestamp to parallel
>> workers, but I can't find any code that does that.  In view of the fact that
>> transaction_timestamp() is marked as parallel-safe, this is a bug in 9.6.

> Yeah.  Do you think we should arrange to pass that down, or change the 
> marking?

We can't fix the marking in existing 9.6 installations, so I think we
have to pass it down.  (Which would be a better response anyway.)

Having said that, I find myself unable to reproduce a problem.
This should fail:

regression=# set parallel_setup_cost TO 0;
SET
regression=# set parallel_tuple_cost TO 0;
SET
regression=# set min_parallel_relation_size TO 0;
SET
regression=# set enable_indexscan TO 0;
SET
regression=# explain verbose select distinct transaction_timestamp() from tenk1;
  QUERY PLAN
  
--
 Unique  (cost=0.00..424.67 rows=1 width=8)
   Output: (transaction_timestamp())
   ->  Gather  (cost=0.00..424.67 rows=1 width=8)
 Output: (transaction_timestamp())
 Workers Planned: 2
 ->  Parallel Seq Scan on public.tenk1  (cost=0.00..410.08 rows=4167 
width=8)
   Output: transaction_timestamp()
(7 rows)

but it doesn't:

regression=# select distinct transaction_timestamp() from tenk1;
 transaction_timestamp 
---
 2016-12-01 15:44:12.839417-05
(1 row)

How is that happening?

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Thu, Dec 1, 2016 at 3:40 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane  wrote:
>>> check-world isn't a magic bullet.
>
>> No, but deliberately leaving things out that could be run isn't
>> helping anything either.
>
> Tests that open security holes while running aren't in my list of "things
> that could be run automatically".

If we create a new target that runs them automatically, you don't have
to use it.

> In any case, you're in a poor position to whine about this given that
> parallel query is entirely unamenable to automated testing, and you've
> resisted past proposals for improving that situation.

Really?

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


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

Re: [HACKERS] Broken SSL tests in master

[Tom Lane]

Robert Haas  writes:
> On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane  wrote:
>> check-world isn't a magic bullet.

> No, but deliberately leaving things out that could be run isn't
> helping anything either.

Tests that open security holes while running aren't in my list of "things
that could be run automatically".

In any case, you're in a poor position to whine about this given that
parallel query is entirely unamenable to automated testing, and you've
resisted past proposals for improving that situation.

regards, tom lane


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Robert Haas]

On Thu, Dec 1, 2016 at 2:32 PM, Tom Lane  wrote:
> I wrote:
>> <5bih4k+4jfl6m39j...@guerrillamail.com> writes:
>>> pg_proc shows that now() is marked as restricted, but 
>>> transaction_timestamp() is marked as safe.
>
>> That's certainly silly, because they're equivalent.  I should think
>> they're both safe.  Robert?
>
> ... well, they would be if we passed down xactStartTimestamp to parallel
> workers, but I can't find any code that does that.  In view of the fact that
> transaction_timestamp() is marked as parallel-safe, this is a bug in 9.6.

Yeah.  Do you think we should arrange to pass that down, or change the marking?

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


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Well, if people are unwilling to add test suites to 'make
>> check-world', we can add 'make check-universe' and I'll run that
>> instead.  And that can come with a big shiny disclaimer.  I just want
>> a way to compile and run EVERYTHING that people care about not
>> breaking, which I think is frankly a pretty reasonable request!
>
> Really?  How are you going to test Windows-specific (or any-other-
> platform-but-yours-specific) code?  How about WORDS_BIGENDIAN code,
> or code that is sensitive to alignment rules?  Or code that breaks
> in locales you haven't got, or depends on compile options you don't
> use?
>
> check-world isn't a magic bullet.

No, but deliberately leaving things out that could be run isn't
helping anything either.

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


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

Re: [HACKERS] PoC: Partial sort

[Peter Geoghegan]

On Mon, Nov 21, 2016 at 11:04 PM, Haribabu Kommi
 wrote:
> you assigned as reviewer to the current patch in the 11-2016 commitfest.
> But you haven't shared your review yet in this commitfest on the latest
> patch posted by the author. If you don't have any comments on the patch,
> please move the patch into "ready for committer" state to get committer's
> attention. This will help us in smoother operation of commitfest.

Sorry for the delay on this.

I agree with Robert's remarks today on TupleTableSlot, and would like
to see a revision that does that.

-- 
Peter Geoghegan


-- 
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] Broken SSL tests in master

[Robert Haas]

On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
> As you can see, after the patch libpq will now look at hostaddr rather than
> host when validating the server certificate because that is what is stored
> in the first (and only) entry of conn->connhost, and therefore what PQhost()
> return.
>
> To me it feels like the proper fix would be to make PQHost() return the
> value of the host parameter rather than the hostaddr (maybe add a new field
> in the pg_conn_host struct). But would be a behaviour change which might
> break someones application. Thoughts?

I think that the blame here is on the original commit,
274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
the behavior of PQhost.  Prior to that commit, even if "hostaddr" was
used, PQhost would still return whatever value was associated with the
"host" parameter, but now it ignores "host" and returns "hostaddr"
instead.  That's busted.  I've pushed a trivial fix, and the SSL tests
now pass for me.

It might be that (as suggested downthread) we should consider
supporting multiple IPs in the hostaddr string as well, but that
requires some thought.  For example, what happens if, for example, the
host and hostaddr lists are of unequal length?  Would we accept one
host and >1 hostaddrs?  Probably makes sense to just apply the host to
every hostaddr.  >1 host and 1 hostaddr?  Probably doesn't make sense,
but I guess you could argue for it.  Equal length lists definitely
make sense.

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


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

Re: [HACKERS] Broken SSL tests in master

[Tom Lane]

Robert Haas  writes:
> Well, if people are unwilling to add test suites to 'make
> check-world', we can add 'make check-universe' and I'll run that
> instead.  And that can come with a big shiny disclaimer.  I just want
> a way to compile and run EVERYTHING that people care about not
> breaking, which I think is frankly a pretty reasonable request!

Really?  How are you going to test Windows-specific (or any-other-
platform-but-yours-specific) code?  How about WORDS_BIGENDIAN code,
or code that is sensitive to alignment rules?  Or code that breaks
in locales you haven't got, or depends on compile options you don't
use?

check-world isn't a magic bullet.

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Tom Lane]

Andres Freund  writes:
> On 2016-12-01 14:43:04 -0500, Robert Haas wrote:
>> I get that, but this is the second time in very recent history that
>> I've broken something because there was code that wasn't compiled or
>> tests that weren't run by 'make check-world'.

> Well, I don't quite know what the alternative is. For some reason, which
> I don't quite understand personally, people care about security during
> regression tests runs. So we can't run the test automatedly.  And nobody
> has added a buildfarm module to run it manually on their servers either
> :(

I don't think there's much substitute for knowing what tests we have
available.

In the particular case at hand, I wonder if we could generate new test
certificates every time (or at least have an option to do that) rather
than relying on premade ones.  But I don't think it's realistic to imagine
that check-world will ever automatedly invoke every possible test.

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Thu, Dec 1, 2016 at 2:45 PM, Andres Freund  wrote:
> Well, I don't quite know what the alternative is. For some reason, which
> I don't quite understand personally, people care about security during
> regression tests runs. So we can't run the test automatedly.  And nobody
> has added a buildfarm module to run it manually on their servers either
> :(

Well, if people are unwilling to add test suites to 'make
check-world', we can add 'make check-universe' and I'll run that
instead.  And that can come with a big shiny disclaimer.  I just want
a way to compile and run EVERYTHING that people care about not
breaking, which I think is frankly a pretty reasonable request!

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


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

Re: [HACKERS] Broken SSL tests in master

[Andres Freund]

On 2016-12-01 14:43:04 -0500, Robert Haas wrote:
> On Thu, Dec 1, 2016 at 2:40 PM, Andres Freund  wrote:
> > On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
> >> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  
> >> wrote:
> >> > The SSL test suite (src/test/ssl) is broken in the master since commit
> >> > 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring 
> >> > of
> >> > getting the server hostname for GSS, SSPI, and SSL in libpq.
> >>
> >> So, we have no buildfarm coverage for this test suite?  And make
> >> check-world doesn't run it?  Sigh.
> >
> > The story behind that is that it opens the server over tcp to everyone
> > who has a copy of our test CA. Which is oh-so-secretly stored in our git
> > repo..
> 
> I get that, but this is the second time in very recent history that
> I've broken something because there was code that wasn't compiled or
> tests that weren't run by 'make check-world'.  I do run that for many
> of my commits even though it takes 15 minutes.  It's frustrating to me
> that it leaves random stuff out and there's no alternative target that
> includes that stuff; I don't like breaking things.  Of course if my
> code reviewing were perfect it wouldn't matter, but I haven't figured
> out how to do that yet.

Well, I don't quite know what the alternative is. For some reason, which
I don't quite understand personally, people care about security during
regression tests runs. So we can't run the test automatedly.  And nobody
has added a buildfarm module to run it manually on their servers either
:(


-- 
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] Broken SSL tests in master

[Robert Haas]

On Thu, Dec 1, 2016 at 2:40 PM, Andres Freund  wrote:
> On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
>> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
>> > The SSL test suite (src/test/ssl) is broken in the master since commit
>> > 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
>> > getting the server hostname for GSS, SSPI, and SSL in libpq.
>>
>> So, we have no buildfarm coverage for this test suite?  And make
>> check-world doesn't run it?  Sigh.
>
> The story behind that is that it opens the server over tcp to everyone
> who has a copy of our test CA. Which is oh-so-secretly stored in our git
> repo..

I get that, but this is the second time in very recent history that
I've broken something because there was code that wasn't compiled or
tests that weren't run by 'make check-world'.  I do run that for many
of my commits even though it takes 15 minutes.  It's frustrating to me
that it leaves random stuff out and there's no alternative target that
includes that stuff; I don't like breaking things.  Of course if my
code reviewing were perfect it wouldn't matter, but I haven't figured
out how to do that yet.

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


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

Re: [HACKERS] Broken SSL tests in master

[Andres Freund]

On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
> > The SSL test suite (src/test/ssl) is broken in the master since commit
> > 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
> > getting the server hostname for GSS, SSPI, and SSL in libpq.
> 
> So, we have no buildfarm coverage for this test suite?  And make
> check-world doesn't run it?  Sigh.

The story behind that is that it opens the server over tcp to everyone
who has a copy of our test CA. Which is oh-so-secretly stored in our git
repo..

Andres


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Tom Lane]

I wrote:
> <5bih4k+4jfl6m39j...@guerrillamail.com> writes:
>> pg_proc shows that now() is marked as restricted, but 
>> transaction_timestamp() is marked as safe.

> That's certainly silly, because they're equivalent.  I should think
> they're both safe.  Robert?

... well, they would be if we passed down xactStartTimestamp to parallel
workers, but I can't find any code that does that.  In view of the fact that
transaction_timestamp() is marked as parallel-safe, this is a bug in 9.6.

regards, tom lane


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

Re: [HACKERS] Broken SSL tests in master

[Robert Haas]

On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson  wrote:
> The SSL test suite (src/test/ssl) is broken in the master since commit
> 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
> getting the server hostname for GSS, SSPI, and SSL in libpq.

So, we have no buildfarm coverage for this test suite?  And make
check-world doesn't run it?  Sigh.

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


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

Re: [HACKERS] Parallel safety of CURRENT_* family

[Tom Lane]

<5bih4k+4jfl6m39j...@guerrillamail.com> writes:
> How should I mark a function which calls CURRENT_DATE? Parallel safe or 
> parallel restricted?

> pg_proc shows that now() is marked as restricted, but transaction_timestamp() 
> is marked as safe.

That's certainly silly, because they're equivalent.  I should think
they're both safe.  Robert?

regards, tom lane


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

Re: [HACKERS] pgbench - allow backslash continuations in \set expressions

[Tom Lane]

Fabien COELHO  writes:
>> In psql, if backslash followed by [CR]LF is interpreted as a
>> continuation symbol, commands like these seem problematic
>> on Windows since backslash is the directory separator:
>> 
>> \cd \
>> \cd c:\somepath\
>> 
>> Shell invocations also come to mind:
>> \! dir \

> Thanks for pointing out these particular cases. I was afraid of such 
> potential issues, hence my questions...

Those look like nasty counterexamples, but I think they are not, because
they don't work today.  What you get is

regression=# \cd \
Invalid command \. Try \? for help.

AFAICT you would need to write it

regression=# \cd '\\'
\cd: could not change directory to "\": No such file or directory

(That's on Unix of course, on Windows I'd expect it to succeed.)

The reason for this is that psql already has a policy that an unquoted
backslash begins a new backslash command on the same line.  Since
there is no command named backslash-return, this is available syntax
that can be repurposed in the direction we want.

I believe that we need to do basically the same thing in pgbench, and
I'm fine with that because I think we should have an overall policy of
synchronizing the psql and pgbench metacommand syntaxes as best we can.
This will at some point necessitate invention of a quoting rule for
pgbench metacommand arguments, so that you can write a backslash as part
of an argument when you need to.  We might not need to do that today
(as I'm not sure there are any use-cases for one), but maybe it'd be best
to just bite the bullet and put it in.

regards, tom lane


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