Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Fri, Jun 8, 2018 at 11:00 PM, Magnus Hagander  wrote:
>
> On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila  wrote:
>>
>>
>> -#include "postgres_fe.h"
>> +#include "postgres.h"
>>
>> I don't think that server application can use backend environment unless
>> it is adapted to do so.  I think the application should have the capability
>> to deal with backend stuff like ereport, elog etc, if we don't want to use
>> FRONTEND environment.  The other server applications like pg_ctl.c,
>> initdb.c, etc. also uses FRONTEND environment.
>>
>
> Not having researched this particular case but yes, there are a number of
> things expected in a non-FRONTEND environment. Things that may also go
> unnoticed until too late (e.g. singal handling etc that needs explicit
> initialization). Standalong binaries should pretty much always be frontend.
>

Right, but here we are facing a situation where using FRONTEND in a
standalone binary (pg_test_fsync) won't accomplish the required
purpose.  Basically, we want to use pgwin32_open (pg specific
implementation for open on windows) in pg_test_fsync and that is
protected by "#ifndef FRONTEND".  As per discussion till now, we have
two options to proceed:
(a) Remove the define "#ifndef FRONTEND" that prevents pgwin32_open
usage in frontend modules.  We have done some research and found that
it was added in the past to allow build of modules like libpq/psql.
If we want to use this option, then some work is needed to ensure all
frontend modules work/behave correctly.
(b) Use c.h in pg_test_fsync which will allow usage of pgwin32_open.

Option (a) appears to be a good approach, but I am not sure what exact
tests would be required.  Do you prefer any of the above or have any
better suggestions?

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

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Andrew Dunstan]

On 06/05/2018 05:33 PM, Andrew Gierth wrote:

"Matthew" == Matthew Woodcraft  writes:

  Matthew> A few days ago there was a suggestion on this list to add a
  Matthew> .editorconfig file specifying tab width:
  Matthew> 
https://www.postgresql.org/message-id/87efhuz9be.fsf%40news-spur.riddles.org.uk

  Matthew> The .editorconfig format also supports a
  Matthew> trim_trailing_whitespace option (see https://editorconfig.org/
  Matthew> ).

Yes. I was actually planning to ask if any .editorconfig users (of which
I'm not one - my only use for it is to improve display output on github)
would like to contribute the most useful content for such a file.




I'm not one either (yet!) but it seems to me like something worth doing. 
It seems to support an impressively large number of editors and display 
mechanisms, and adding a single small file to support it doesn't seem 
onerous.


cheers

andrew

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

Re: hot_standby_feedback vs excludeVacuum and snapshots

[Amit Kapila]

On Fri, Jun 8, 2018 at 5:27 PM, Simon Riggs  wrote:
> On 8 June 2018 at 11:33, Amit Kapila  wrote:
>> On Fri, Jun 8, 2018 at 1:53 PM, Simon Riggs  wrote:
>>>
>>> So the attached patch fixes both the bug in the recent commit and the
>>> one I just found by observation of 49bff5300d527, since they are
>>> related.
>>>
>>> StandbyReleaseOldLocks() can sweep in the same way as
>>> ExpireOldKnownAssignedTransactionIds().
>>>
>>
>
>> How will this avoid the bug introduced by your recent commit
>> (32ac7a11)?  After that commit, we no longer consider vacuum's xid in
>> RunningTransactions->oldestRunningXid calculation, so it is possible
>> that we still remove/release its lock on standby earlier than
>> required.
>
> In the past, the presence of an xid was required to prevent removal by
> StandbyReleaseOldLocks().
>
> The new patch removes that requirement and removes when the xid is
> older than oldestxid
>

The case I have in mind is: "Say vacuum got xid 600 while truncating,
and then some other random transactions 601,602  starts modifying the
database.  At this point, I think the computed value of
RunningTransactions->oldestRunningXid will be 601.  Now, on standby
when StandbyReleaseOldLocks sees the lock from transaction 600, it
will remove it which doesn't appear correct to me.".

> The normal path of removal is at commit or abort,
> StandbyReleaseOldLocks is used almost never (as originally intended).
>

Okay, but that doesn't prevent us to ensure that whenever used, it
does the right thing.

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

Re: why partition pruning doesn't work?

[Tom Lane]

I wrote:
> One thing I'm wondering about is why in the world are PartitionPruneInfo
> and its subsidiary struct types declared in primnodes.h?

Oh, and while I'm bitching: it seems like there is hardly any part of
the partitioning code in which the comments aren't desperately in need
of a copy-editing pass.  They are just chock-full of misspellings,
grammar that is faulty enough to make the meaning unclear, and/or
errors of fact.  An example of the latter is the repeated claims that
the basic partitioning functions belong to the planner.  Maybe that
was true at some stage of development; but AFAICS the logic in question
now lives in src/backend/partitioning/, which I would not think is
part of the planner.

regards, tom lane

Re: POC: GROUP BY optimization

[Tomas Vondra]

On 06/09/2018 08:09 PM, Tomas Vondra wrote:
> 
> /snip/
> 
> 4) when adding Sort for grouping, try producing the right output order
>(if the ORDER BY was specified)
> 

BTW I've just realized we already do something similar in master. If you
run a query like this:

  SELECT a, b, count(*) FROM t GROUP BY b, a ORDER BY a;

we will actually plan it like this:

  QUERY PLAN
  ---
   GroupAggregate
 Group Key: a, b
 ->  Sort
   Sort Key: a, b
   ->  Seq Scan on t
  (5 rows)

I.e. we already do reorder the group clauses to match ORDER BY, to only
require a single sort. This happens in preprocess_groupclause(), which
also explains the reasoning behind that.

I wonder if some of the new code reordering group pathkeys could/should
be moved here (not sure, maybe it's too early for those decisions). In
any case, it might be appropriate to update some of the comments before
preprocess_groupclause() which claim we don't do certain things added by
the proposed patches.

This probably also somewhat refutes my claim that the order of grouping
keys is currently fully determined by users (and so they may pick the
most efficient order), while the reorder-by-ndistinct patch would make
that impossible. Apparently when there's ORDER BY, we already mess with
the order of group clauses - there are ways to get around it (subquery
with OFFSET 0) but it's much less clear.

regards

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

Re: adding tab completions

[Justin Pryzby]

Thanks for review and comment.

On Tue, Jun 05, 2018 at 05:29:42PM +0300, Arthur Zakirov wrote:
> On Sun, Jun 03, 2018 at 10:39:22PM -0500, Justin Pryzby wrote:

> > > Also I think it could be good to list column names after parentheses,
> > > but I'm not sure if it easy to implement.
> > I tried this and nearly gave up, but see attached.
> 
> After some thought now I think that this is not so good idea. The code
> doesn't look good too. It doesn't worth it and sorry for the distraction.

No problem.

> Moreover there is no such completion for example for the command (it shows
> only first column):
> 
> CREATE INDEX ON test (

Noted (I misunderstood at first: you just mean there's precedent that column
names aren't completed, except maybe the first).

I can revise patch to not complete attributes in analyze; but, I think that
means that this will have to complete to table names:

postgres=# ANALYZE tt (a , 
alu_enodeb_201601 information_schema.
alu_enodeb_201602 jrn_pg_buffercache
...

.. since, without checking for matching parens, it has no idea whether to
complete with rels or atts.  WDYT?

Note that HeadMatch functions have special behavior with matching parenthesis:
if previous char is an right parenthesis, then the "previous word" is taken to
be the entire parenthesized value.  Maybe there's more that I don't know, but I
can't see how that can be easily applied here (by easily I mean without doing
something like making a temp copy of the buffer and inserting an "exploratory"
right parenthesis to see if TailMatches(prev_wd, ')') && prev_wd[0]=='(' or
similar).

An alternative is that only the first table is completed for vacuum/analyze.

CREATE INDEX should complete columns, too.  It has the "ON" keyword (which
makes trying to parse less fragile).

> > -   "SERVER", "INDEX", "LANGUAGE", "POLICY", "PUBLICATION", 
> > "RULE",
> > +   "SERVER", "INDEX", "LANGUAGE", "POLICY", "PUBLICATION",
> 
> Is this a typo? I think still there is a possibility to comment rules.

Not in PG11(b1) (note, that's a custom table)
postgres=# COMMENT ON RULE pg_settings_u IS 'asdf';
ERROR:  syntax error at or near "IS"

I see I didn't include that description in my first mail.

Here's a description and possible commit message for the (still WIP) patch:

Update psql tab completion for commits:

Table completion for ANALYZE partitioned_table
3c3bb99330aa9b4c2f6258bfa0265d806bf365c3

Add parenthesized options syntax for ANALYZE.
854dd8cff523bc17972d34772b0e39ad3d6d46a4

Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.
ede62e56fbe809baa1a7bc3873d82f12ffe7540b

Allow multiple tables to be specified in one VACUUM or ANALYZE command.
11d8d72c27a64ea4e30adce11cf6c4f3dd3e60db

Remove deprecated COMMENT ON RULE syntax
e8d016d81940e75c126aa52971b7903b7301002e

Add hash partitioning.
1aba8e651ac3e37e1d2d875842de1e0ed22a651e

Parameter toast_tuple_target
c2513365a0a85e77d3c21adb92fe12cfbe0d1897

Parenthesized explain (...)
d4382c4ae7ea1e272f4fee388aac8ff99421471a

Parameter toast_tuple_target controls TOAST for new rows
c2513365a0a85e77d3c21adb92fe12cfbe0d1897

no longer accepts VACUUM ANALYZE VERBOSE
921059bd66c7fb1230c705d3b1a65940800c4cbb

> > else if (HeadMatches1("EXPLAIN") && previous_words_count==2 && 
> > prev_wd[0]=='(' && ends_with(prev_wd, ')'))
> 
> I think this condition can be replaced by:
> 
> else if (TailMatches2("EXPLAIN", MatchAny) && ends_with(prev_wd, ')'))
> 
> It looks better to me. Such condition is used for other commands and
> works the same way.
> 
> The last point I've noticed, there is no VERBOSE entry after VACUUM FULL
> ANALYZE command anymore.

See commit 921059bd6, above (it's not 100% clear to me that's intended to
reject VACUUM ANALYZE VERBOSE and not just reject VACUUM VERBOSE ANALYZE
VERBOSE, but I tentatively assume it's intentional).

> I'm not sure how this patch should be commited. Can it be commited
> outside the commitfest? Otherwise add it to the next commitfest please
> in order not to forget it.

I've done https://commitfest.postgresql.org/18/1661/

Justin

PostgreSQL vs SQL Standard

[Andrew Gierth]

I created this wiki page:

https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL_Standard

I'd been thinking of collecting this information for a while, but was
spurred into further action when someone referred me to Markus Winand's
PGCon talk slides.

I think I got all the issues I currently know of, but there may be
more, and others may disagree with my classification of issues or the
rationales for violating the spec. Any feedback?

-- 
Andrew (irc:RhodiumToad)

Re: PostgreSQL vs SQL Standard

[Tom Lane]

Andrew Gierth  writes:
> I created this wiki page:
> https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL_Standard

Good idea!

> I think I got all the issues I currently know of, but there may be
> more, and others may disagree with my classification of issues or the
> rationales for violating the spec. Any feedback?

WRT 1.1 ... I doubt that redefining DROP DOMAIN as you describe has "no
major issues".  It sounds to me like an incredibly ugly wart on the
cascaded dependency logic.  Quite aside from wartiness, adding new
objects/dependencies as part of a DROP is broken by design.  What if
the domain drop has cascaded from something the domain's constraints
themselves depend on?  I'd put this as a "has design-level problems" item.

WRT 3.2 on select-list aliases, the postfix-operator issue is only one of
several reasons why we can't support that.  There was some more-detailed
discussion about that awhile back,
https://www.postgresql.org/message-id/flat/99ad0450-b1ab-702f-48ef-6972b630bc87%40BlueTreble.com

Constraint name scope: I think it's an overstatement to say that this
makes some info-schema views "useless".  "Under-determined" might be an
appropriate word.  Or you could say "useless unless the application
limits itself to follow the SQL spec's restriction on names".

Object ownership scope: I have not really dug into the spec on this
point, but I recall from our own docs that "schema owner owns all
contained objects too" is true only in DBs that implement some minimal
subset of the standard.  So that might need more explanation.  In
any case, we can surely mount a very strong defense in terms of
usability/flexibility here, we don't need to say it's just historical.

regards, tom lane

Re: Add CONTRIBUTING.md

[Peter Geoghegan]

On Tue, May 29, 2018 at 10:10 AM, Andres Freund  wrote:
>> WHile you're at it, you could also add an .editorconfig so that github
>> displays code with 4-character tabs.
>>
>> (e.g. https://github.com/RhodiumToad/pllua-ng/blob/master/.editorconfig  )
>
> +1 - why don't you start a thread about it? I'd actually even suggest
> backpatching it.

PostGIS has one of these too, which covers several languages,
including SQL. Perhaps it would be worth tightening up the indentation
of SQL files while we're at it.

-- 
Peter Geoghegan

Re: Postgres 11 release notes

[Justin Pryzby]

On Sat, Jun 09, 2018 at 01:35:19PM -0700, David G. Johnston wrote:
> On Fri, May 11, 2018 at 8:08 AM, Bruce Momjian  wrote:
> 
> > I have committed the first draft of the Postgres 11 release notes.  I
> > will add more markup soon.  You can view the most current version here:
> >
> > http://momjian.us/pgsql_docs/release-11.html
> 
> """
> Also, if any table mentioned in VACUUM uses a column list, then ANALYZE
> keyword must be supplied; previously ANALYZE was implied in such cases.
> """
> 
> Should that be mentioned in the compatibility notes?

+1

Note also from 921059bd66c7fb1230c705d3b1a65940800c4cbb 


This is okay in v10 but rejected in v11b1:  

  
postgres=# VACUUM ANALYZE VERBOSE t;

  
ERROR:  syntax error at or near "VERBOSE"   

  

Justin

Re: Explain buffers wrong counter with parallel plans

[Amit Kapila]

On Fri, Jun 8, 2018 at 11:34 PM, Robert Haas  wrote:
> On Wed, Jun 6, 2018 at 12:08 PM, Alvaro Herrera
>  wrote:
>> So, apparently this is not a Postgres 11 open item, but rather a bug
>> that goes back to pg10.  However, maybe it would be worth fixing soon
>> anyway?  In particular, if we want to perturb the explain output as
>> suggested upthread, maybe *that* should be considered an open item?
>>
>> Anyway, this has stalled for a month now.
>
> Yeah.  I'm not willing to do anything here unilaterally.  There is
> neither universal agreement that there is a problem here nor agreement
> on a fix.
>

Right, I think we have following options:
(a) Come up with a solution which allows percolating the buffer usage
and or similar stats to upper nodes in all cases.
(b) Allow it to work for some of the cases as it was earlier.

I think (b) can cause confusion and could lead to further questions on
which specific cases will it work and for which it won't work.  If we
think (a) is a reasonable approach, then we can close this item with a
conclusion as a work item for future and OTOH if we think option (b)
is the better way to deal with it, then we can come up with a patch to
do so.  My inclination is to go with option (a), but I don't mind if
the decision is to choose option (b).

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

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

Hello Marina,


v9-0002-Pgbench-errors-use-the-Variables-structure-for-cl.patch
- a patch for the Variables structure (this is used to reset client variables 
during the repeating of transactions after serialization/deadlock failures).


About this second patch:

This extracts the variable holding structure, so that it is somehow easier 
to reset them to their initial state on transaction failures, the 
management of which is the ultimate aim of this patch series.


It is also cleaner this way.

Patch applies cleanly on top of the previous one (there is no real 
interactions with it). It compiles cleanly. Global & pgbench "make check" 
are both ok.


The structure typedef does not need a name. "typedef struct { } V...".

I tend to disagree with naming things after their type, eg "array". I'd 
suggest "vars" instead. "nvariables" could be "nvars" for consistency with 
that and "vars_sorted", and because "foo.variables->nvariables" starts 
looking heavy.


I'd suggest but "Variables" type declaration just after "Variable" type 
declaration in the file.


--
Fabien.

Re: JIT documentation fixes

[Peter Geoghegan]

On Thu, May 31, 2018 at 11:50 AM, Daniel Gustafsson  wrote:
> When reading the JIT developer documentation, a few small wordsmithing issues
> stood out (although this may be due to me not being a native english speaker).
> The attached patch fixes these to what I think the sentences inteded to say.

Committed with a few adjustments.

I found that "an SQL..." was about as common as "a SQL..." in the
documentation. Either can be correct -- it depends on how you
pronounce "SQL". I didn't see any point in those changes, perhaps
because I always say "S-Q-L".

I'm also pretty sure that Andres did in fact mean "...even for faster
queries", since an LRU cache of JIT functions should be particularly
useful for OLTP queries that are already individually fast enough to
make per-execution JIT compilation overhead prohibitively expensive.
That change was also left out. If I'm mistaken in how I interpreted
the sentence, then Andres should follow up.

Thanks
-- 
Peter Geoghegan

Re: POC: GROUP BY optimization

[Tomas Vondra]

On 06/07/2018 06:22 PM, Teodor Sigaev wrote:
>>> Yes. But again, this description is a bit short. First it works after
>>> first patch and might get some preordered leading pathkeys. Second, it
>>> tries to match ORDER BY clause order if there is no preordered leading
>>> pathkeys from first patch (it was introduced in v7). And third, if there
>>> is a tail of unmatched pathkeys on previous stages then it will reorder
>>> that tail.
>>>
>>
>> OK, I haven't looked at v7 yet, but if I understand correctly it tries
>> to maintain the ordering as much as possible? Does that actually help? I
>> mean, the incremental sort patch allows the sorting to happen by pieces,
>> but here we still need to sort all the data, right?
>>
>> Can you give an example demonstrating the benefit?
> 
> See tst.sql. queries are marked with opt (optimization is on) and noopt.
> 
> Query 1: select count(*) from btg group by v, r;
> Query 2: select count(*) from btg group by n, v, r order by n;
> 
> For both queries it's possible to reorder v and r column, n column has
> the single distinct value.
> 
> On my laptop:
> Query 1opt vs 1noopt: 3177.500 ms vs 6604.493 ms
>   2opt vs 2noopt: 5800.307 ms vs 7486.967 ms
> 
> So, what we see:
> 1) for query 1 optimization gives 2 times better performance, for query
> 2 only 30%. if column 'n' will be unique then time for query 1 and 2
> should be the same. We could add check for preordered pathkeys in
> get_cheapest_group_keys_order() and if estimate_num_groups(reordered
> pathkeys) is close to 1 then we could do not reordering of tail of
> pathkeys.
> 

OK, those are nice improvements, although the examples are somewhat
extreme (only 1 or 2 distinct values). So yes, in some cases this
reordering clearly makes a big difference, but I still think we need to
improve the heuristics to minimize regressions.

I see v9 is now calling estimate_num_groups(), so it already benefits
from extended statistics. Nice! I think the algorithm is more or less
the greedy option I proposed in one of the earlier messages - I don't
know if it's sufficient or not, but the only other idea I have is
essentially an exhaustive search through all possible permutations.

So that's a nice improvement, although I think we should also consider
non-uniform distributions (using the per-column MCV lists).

> 2) Planing cost is the same for all queries. So, cost_sort() doesn't
> take into account even number of columns.
> 

Yeah. As I wrote before, I think we'll need to come up with a model for
comparison_cost depending on the column order, which should make costs
for those paths different.

>> FWIW I think it would be useful to have "development GUC" that would
>> allow us to enable/disable these options during development, because
>> that makes experiments much easier. But then remove them before commit.
> Added, v9, debug_enable_group_by_match_order_by and
> debug_enable_cheapest_group_by. I also checked compatibility with
> incremental sort patch, and all works except small merge conflict which
> could be resolved right before committing.
> 

OK. I think we should also have a debug GUC for the first patch (i.e.
one that would allow reordering group_pathkeys to match the input path).

Regarding the two GUCs introduced in v9, I'm not sure I 100% understand
what they are doing. Let me try explaining what I think they do:

1) debug_cheapest_group_by - Reorder GROUP BY clauses to using ndistinct
stats for columns, placing columns with more distinct values first (so
that we don't need to compare the following columns as often).

2) debug_group_by_match_order_by - Try to reorder the GROUP BY clauses
to match the ORDER BY, so that the group aggregate produces results in
the desired output (and we don't need an explicit Sort).

FWIW I doubt about the debug_group_by_match_order_by optimization. The
trouble is that it's only based on comparing the pathkeys lists, and
entirely ignores that the two result sets may operate on very different
numbers of rows.

Consider for example "SELECT * FROM t GROUP BY a,b,c,d ORDER BY c,d"
where table "t" has 1B rows, but there are only ~10k rows in the result
(which is fairly common in fact tables in star-schema DBs). IIUC the
optimization will ensure "c,d" is placed first in the GROUP BY, and then
we reorder "a,b" using ndistinct. But this may be much less efficient
than simply reordering (a,b,c,d) per ndistinct and then adding explicit
Sort on top of that (because on 10k rows that's going to be cheap).

So I think this somewhat contradicts the order-by-ndistinct optimization
and may easily undo it's benefits.

The other "issue" I have with get_cheapest_group_keys_order() is how it
interacts with group_keys_reorder_by_pathkeys(). I mean, we first call

  n_preordered = group_keys_reorder_by_pathkeys(path->pathkeys, ...);

so the reordering tries to maintain ordering of the input path. Then if
(n_preordered == 0) we do this:

  group_keys_reorder_by_pathkeys(root->sort_pathkeys, ...);

Doesn't 

Re: JIT documentation fixes

[Daniel Gustafsson]

> On 9 Jun 2018, at 19:05, Peter Geoghegan  wrote:
> 
> On Thu, May 31, 2018 at 11:50 AM, Daniel Gustafsson  wrote:
>> When reading the JIT developer documentation, a few small wordsmithing issues
>> stood out (although this may be due to me not being a native english 
>> speaker).
>> The attached patch fixes these to what I think the sentences inteded to say.
> 
> Committed with a few adjustments.

Thanks, I’m honoured to get the first commit.

> I'm also pretty sure that Andres did in fact mean "...even for faster
> queries", since an LRU cache of JIT functions should be particularly
> useful for OLTP queries that are already individually fast enough to
> make per-execution JIT compilation overhead prohibitively expensive.

Ah, I completely misunderstood that part (apparently) but that make sense.

cheers ./daniel

Re: why partition pruning doesn't work?

[Tom Lane]

David Rowley  writes:
> I'm really hoping this is what you meant about the special-case code for 
> Params.
> Does this look any better?

I'm starting to look this over and it seems like generally the right
thing, though I'm finding minor things I don't like much.

One thing I'm wondering about is why in the world are PartitionPruneInfo
and its subsidiary struct types declared in primnodes.h?  They are not
general-purpose expression nodes, or if they are then there are an awful
lot of places that should know about them and do not.  AFAICT they might
belong in plannodes.h.

regards, tom lane

Re: Postgres 11 release notes

[David G. Johnston]

On Fri, May 11, 2018 at 8:08 AM, Bruce Momjian  wrote:

> I have committed the first draft of the Postgres 11 release notes.  I
> will add more markup soon.  You can view the most current version here:
>
> http://momjian.us/pgsql_docs/release-11.html


​Some thoughts:​

​As a major ​item:

JIT compilation of some SQL code, including support for fast evaluation of
expressions


leaves me wondering what the general benefit is to the user.  Also,
spelling out "Just-In-Time (JIT)" here seems warranted.


"""
No longer retain WAL that spans two checkpoints (Simon Riggs)

The retention of WAL records for only one checkpoint is required.
"""

Maybe: "Servers now only ensure that a single checkpoint record is present
in the locally retained WAL.  Previously it would ensure at least two
checkpoints were available for recovery."


"""
Also, if any table mentioned in VACUUM uses a column list, then ANALYZE
keyword must be supplied; previously ANALYZE was implied in such cases.
"""

Should that be mentioned in the compatibility notes?


"""
Add the ability to define PL/pgSQL record types as not null, constant, or
with initial values (Tom Lane)
"""

Found the commit message for this one - should we back-patch a
documentation change indicating that this doesn't work prior to v11?

David J.

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 09-06-2018 16:31, Fabien COELHO wrote:

Hello Marina,


Hello!


v9-0002-Pgbench-errors-use-the-Variables-structure-for-cl.patch
- a patch for the Variables structure (this is used to reset client 
variables during the repeating of transactions after 
serialization/deadlock failures).


About this second patch:

This extracts the variable holding structure, so that it is somehow
easier to reset them to their initial state on transaction failures,
the management of which is the ultimate aim of this patch series.

It is also cleaner this way.

Patch applies cleanly on top of the previous one (there is no real
interactions with it). It compiles cleanly. Global & pgbench "make
check" are both ok.


:-)


The structure typedef does not need a name. "typedef struct { } V...".


Ok!


I tend to disagree with naming things after their type, eg "array".
I'd suggest "vars" instead. "nvariables" could be "nvars" for
consistency with that and "vars_sorted", and because
"foo.variables->nvariables" starts looking heavy.

I'd suggest but "Variables" type declaration just after "Variable"
type declaration in the file.


Thank you, I agree and I'll fix all this.

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Compromised postgresql instances

[Andrew Dunstan]

On 06/09/2018 03:27 AM, Andrew Gierth wrote:

"Thomas" == Thomas Kellerer  writes:

  Thomas> And a blog post going into details on how that specific attack works.

  Thomas> 
https://www.imperva.com/blog/2018/03/deep-dive-database-attacks-scarlett-johanssons-picture-used-for-crypto-mining-on-postgre-database/

*headdesk*

*headdesk*

*headdesk*

FOR THE LOVE OF LITTLE APPLES, why, in an article as comprehensive as
this, did they not list in the "quick tips" at the end, the quickest and
most absolutely basic and essential tip of all, which is "don't open up
your database for superuser access from the whole world" ???

To become vulnerable to this attack, you have to do ALL of these:

   - give your db a public IP
   - allow access (or forget to prevent access) to it through any
 firewall
   - configure pg to listen on the public IP
   - explicitly add an entry to pg_hba.conf that allows access from
 0.0.0.0/0 for all users or at least the postgres user
   - AND have a guessable password on the postgres user or explicitly
 use "trust" on the above hba entry

*headdesk*




Against stupidity the Gods themselves contend in vain.

cheers

andrew

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

Re: Postgres 11 release notes

[Jonathan S. Katz]

> On Jun 9, 2018, at 4:35 PM, David G. Johnston  
> wrote:
> 
> On Fri, May 11, 2018 at 8:08 AM, Bruce Momjian  > wrote:
> I have committed the first draft of the Postgres 11 release notes.  I
> will add more markup soon.  You can view the most current version here:
> 
> http://momjian.us/pgsql_docs/release-11.html 
> 
> 
> ​Some thoughts:​
> 
> ​As a major ​item:
> 
> JIT compilation of some SQL code, including support for fast evaluation of 
> expressions
> 
> 
> leaves me wondering what the general benefit is to the user.  Also, spelling 
> out "Just-In-Time (JIT)" here seems warranted.

+1 for spelling it out.

Looking through past release notes[1][2], we’ve typically left the context for
the features press releases[3].

Now, as the beta press release (feature-explanation oriented) typically differs 
from
the major version press release (use-case/context oriented), perhaps we could
incorporate some of the elements from the beta press release into the
“Major Features” section. It would be a departure from the release notes of 
recent
memory, but perhaps it would be more helpful.

Thoughts?

Jonathan

[1] https://www.postgresql.org/docs/11/static/release-10.html 

[2] https://www.postgresql.org/docs/11/static/release-9-6.html 

[3] https://www.postgresql.org/about/news/1855/ 

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 09-06-2018 9:55, Fabien COELHO wrote:

Hello Marina,


Hello!


v9-0001-Pgbench-errors-use-the-RandomState-structure-for-.patch
- a patch for the RandomState structure (this is used to reset a 
client's random seed during the repeating of transactions after 
serialization/deadlock failures).


A few comments about this first patch.


Thank you very much!


Patch applies cleanly, compiles, global & pgbench "make check" ok.

I'm mostly ok with the changes, which cleanly separate the different
use of random between threads (script choice, throttle delay,
sampling...) and client (random*() calls).


Glad to hear it :)


This change is necessary so that a client can restart a transaction
deterministically (at the client level at least), which is the
ultimate aim of the patch series.

A few remarks:

The RandomState struct is 6 bytes, which will induce some padding when
used. This is life and pre-existing. No problem.

ISTM that the struct itself does not need a name, ie. "typedef struct
{ ... } RandomState" is enough.


Ok!


There could be clear comments, say in the TState and CState structs,
about what randomness is impacted (i.e. script choices, etc.).


Thank you, I'll add them.


getZipfianRand, computeHarmonicZipfian: The "thread" parameter was
justified because it was used for two fieds. As the random state is
separated, I'd suggest that the other argument should be a zipfcache
pointer.


I agree with you and I will change it.


While reading your patch, it occurs to me that a run is not
deterministic at the thread level under throttling and sampling,
because the random state is sollicited differently depending on when
transaction ends. This suggest that maybe each thread random_state use
should have its own random state.


Thank you, I'll fix this.


In passing, and totally unrelated to this patch:

I've always been a little puzzled about why a quite small 48-bit
internal state random generator is used. I understand the need for pg
to have a portable & state-controlled thread-safe random generator,
but why this particular small one fails me. The source code
(src/port/erand48.c, copyright in 1993...) looks optimized for 16 bits
architectures, which is probably pretty inefficent to run on 64 bits
architectures. Maybe this could be updated with something more
consistent with today's processors, providing more quality at a lower
cost.


This sounds interesting, thanks!
*went to look for a multiplier and a summand that are large enough and 
are mutually prime..*


--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: hot_standby_feedback vs excludeVacuum and snapshots

[Simon Riggs]

On 8 June 2018 at 19:03, Andres Freund  wrote:

>> It seems to have affected Greg.
>
> As far as I can tell Greg was just theorizing?

I'll wait for Greg to say whether this was an actual case that needs
to be fixed or not. If not, happy to revert.

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

Re: hot_standby_feedback vs excludeVacuum and snapshots

[Simon Riggs]

On 8 June 2018 at 19:03, Andres Freund  wrote:
> Hi,
>
> On 2018-06-08 09:23:02 +0100, Simon Riggs wrote:
>> I have also found another bug which affects what we do next.
>>
>> For context, AEL locks are normally removed by COMMIT or ABORT.
>> StandbyReleaseOldLocks() is just a sweeper to catch anything that
>> didn't send an abort before it died, so it hardly ever activates. The
>> coding of StandbyReleaseOldLocks() is backwards... if it ain't in the
>> running list, then we remove it.
>>
>> But that wasn't working correctly either, since as of 49bff5300d527 we
>> assigned AELs to subxids. Subxids weren't in the running list and so
>> AELs held by them would have been removed at the wrong time, an extant
>> bug in PG10. It looks to me like they would have been removed either
>> early or late, up to the next runningxact info record. They would be
>> removed, so no leakage, but the late timing wouldn't be noticeable for
>> tests or most usage, since it would look just like lock contention.
>> Early release might give same issue of block access to non-existent
>> block/file.
>
> Yikes, that's kinda bad. It can also just cause plain crashes, no? The
> on-disk / catalog state isn't necessarily consistent during DDL, which
> is why we hold AE locks. At the very least it can cause corruption of
> in-use relcache entries and such.  In practice the fact this probably
> hits only a limited number of people because it requires DDL to happen
> in subtransactions, which probably isn't *that* common.

Yep, kinda bad, hence fix.

>> So the attached patch fixes both the bug in the recent commit and the
>> one I just found by observation of 49bff5300d527, since they are
>> related.
>
> Can we please keep them separate?

The attached patch is separate. It fixes 49bff5300d527 and also the
committed code, but should work fine even if we revert. Will
doublecheck.

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

Re: Internal error XX000 with enable_partition_pruning=on, pg 11 beta1 on Debian

[Tom Lane]

David Rowley  writes:
> So it looks like I've assumed that the Append path's partitioned_rels
> will only ever be set for partitioned tables, but it can, in fact, be
> set for UNION ALL parents too when the union children are partitioned
> tables.

> As a discussion topic, I've attached a patch which does resolve the
> error, but it also disables run-time pruning in this case.

> There might be some way we can treat UNION ALL parents differently
> when building the PartitionPruneInfos. I've just not thought of what
> this would be just yet. If I can't think of that, I wonder if this is
> a rare enough case not to bother with run-time partition pruning.

So, IIUC, the issue is that for partitioning cases Append expects *all*
its children to be partitions of the *same* partitioned table?  That
is, you could also break it with

select * from partitioned_table_a
union all
select * from partitioned_table_b

?

If so, maybe the best solution is to not allow a partitioning appendrel
to be flattened into an appendrel generated in other ways (particularly,
via UNION ALL).  I also wonder whether it was a bad idea to treat these
as the same kind of path/plan in the first place.

regards, tom lane

Re: JIT documentation fixes

[Peter Geoghegan]

On Sat, Jun 9, 2018 at 10:40 AM, Daniel Gustafsson  wrote:
> Thanks, I’m honoured to get the first commit.

Keep them coming.   :-)

-- 
Peter Geoghegan

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

Hello Marina,


v9-0001-Pgbench-errors-use-the-RandomState-structure-for-.patch
- a patch for the RandomState structure (this is used to reset a client's 
random seed during the repeating of transactions after serialization/deadlock 
failures).


A few comments about this first patch.

Patch applies cleanly, compiles, global & pgbench "make check" ok.

I'm mostly ok with the changes, which cleanly separate the different use 
of random between threads (script choice, throttle delay, sampling...) and 
client (random*() calls).


This change is necessary so that a client can restart a transaction 
deterministically (at the client level at least), which is the ultimate 
aim of the patch series.


A few remarks:

The RandomState struct is 6 bytes, which will induce some padding when 
used. This is life and pre-existing. No problem.


ISTM that the struct itself does not need a name, ie. "typedef struct { 
... } RandomState" is enough.


There could be clear comments, say in the TState and CState structs, about 
what randomness is impacted (i.e. script choices, etc.).


getZipfianRand, computeHarmonicZipfian: The "thread" parameter was 
justified because it was used for two fieds. As the random state is 
separated, I'd suggest that the other argument should be a zipfcache 
pointer.


While reading your patch, it occurs to me that a run is not deterministic 
at the thread level under throttling and sampling, because the random 
state is sollicited differently depending on when transaction ends. This 
suggest that maybe each thread random_state use should have its own random 
state.


In passing, and totally unrelated to this patch:

I've always been a little puzzled about why a quite small 48-bit internal 
state random generator is used. I understand the need for pg to have a 
portable & state-controlled thread-safe random generator, but why this 
particular small one fails me. The source code (src/port/erand48.c, 
copyright in 1993...) looks optimized for 16 bits architectures, which is 
probably pretty inefficent to run on 64 bits architectures. Maybe this 
could be updated with something more consistent with today's processors, 
providing more quality at a lower cost.


--
Fabien.

Re: Compromised postgresql instances

[Andrew Gierth]

> "Thomas" == Thomas Kellerer  writes:

 Thomas> And a blog post going into details on how that specific attack works.

 Thomas> 
https://www.imperva.com/blog/2018/03/deep-dive-database-attacks-scarlett-johanssons-picture-used-for-crypto-mining-on-postgre-database/

*headdesk*

*headdesk*

*headdesk*

FOR THE LOVE OF LITTLE APPLES, why, in an article as comprehensive as
this, did they not list in the "quick tips" at the end, the quickest and
most absolutely basic and essential tip of all, which is "don't open up
your database for superuser access from the whole world" ???

To become vulnerable to this attack, you have to do ALL of these:

  - give your db a public IP
  - allow access (or forget to prevent access) to it through any
firewall
  - configure pg to listen on the public IP
  - explicitly add an entry to pg_hba.conf that allows access from
0.0.0.0/0 for all users or at least the postgres user
  - AND have a guessable password on the postgres user or explicitly
use "trust" on the above hba entry

*headdesk*

-- 
Andrew (irc:RhodiumToad)