Re: [HACKERS] GUC for cleanup indexes threshold.

[Masahiko Sawada]

On Wed, Feb 28, 2018 at 1:45 AM, Alexander Korotkov
 wrote:
> On Wed, Nov 29, 2017 at 6:06 PM, Masahiko Sawada 
> wrote:
>>
>> On Wed, Nov 29, 2017 at 11:05 PM, Simon Riggs 
>> wrote:
>> > On 25 September 2017 at 22:34, Kyotaro HORIGUCHI
>> >  wrote:
>> >
>> >>> > Here is a small patch that skips scanning btree index if no pending
>> >>> > deleted pages exists.
>> >>> > It detects this situation by comparing pages_deleted with
>> >>> > pages_free.
>> >>
>> >> It seems to work to prevent needless cleanup scans.
>> >
>> > So this leaves us in the situation that
>> >
>> > 1. Masahiko's patch has unresolved problems
>> > 2. Yura's patch works and is useful
>> >
>> > Unless there is disagreement on the above, it seems we should apply
>> > Yura's patch (an edited version, perhaps).
>> >
>>
>> IIRC the patches that makes the cleanup scan skip has a problem
>> pointed by Peter[1], that is that we stash an XID when a btree page is
>> deleted, which is used to determine when it's finally safe to recycle
>> the page. Yura's patch doesn't have that problem?
>>
>> [1]
>> https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com
>
>
> Yes, I think Yura's patch doesn't have that problem, because it skips
> cleanup only when there are no recyclable pages.  And that means there
> is no btpo.xact stored, so no XIDs to be wraparounded.

I've looked at the patch again. And you're right, Yura's patch doesn't
have that problem.

>
> BTW, I've rebased Yura's patch.  I think this patch has following issues
> for now:
>
> 1) Adding BTP_NEED_NO_CLEANUP as per-page flag doesn't look nice.

Yeah, the alternative ideas are to store the flag it into pd_rune_xid
of meta page or to use 1bit of btm_version so that we don't break disk
format compatibility.

> 2) In the append-only case, index statistics can lag indefinitely.

The original proposal proposed a new GUC that specifies a fraction of
the modified pages to trigger a cleanup indexes. Combining with Yura's
patch and telling requirement of cleanup to indexes from lazyvacuum
code we can deal with it.

Regards,

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

Re: 2018-03 Commitfest Summary (Andres #4)

[Andres Freund]

Hi,

On 2018-03-01 20:34:11 -0800, Andres Freund wrote:
> On 2018-03-01 14:45:15 -0800, Andres Freund wrote:
> > Second round.
>
> And last round. [ work ]. Scratch that. There'll be one more after this
> ;)

Let's do this. Hell, this CF is large.  I'll have a glass of wine at
some point of this.

- Add default role pg_access_server_files

  WOA. For a couple weeks now.  Should kind abe punted, but I think the
  amount of changes required isn't that large...


- foreign keys and partitioned tables

  NR, but should probably be WOA.

  This still seems fairly rough, hasn't been reviewed etc.  I don't see
  this going in unfortunately.


- Predicate locking in gin index

  NR.

  I don't quite think this is going in v11. There's not been much
  review, the path is nontrivial.


- SQL/JSON: jsonpath

  NR, could also bo WOA.

  I don't see this realistically targeting v11. There's been very
  little code review of the main patch.   It's a bit sad that this got
  kickstarted quite late, after people argued about getting an earlier
  version of this into last release...


- Refuse setting toast.* reloptions when TOAST relation does not exist

  NR.  I think the conclusion here is that we're not sure what we want
  to do.  I'd reject and let the issue live for another day.


- Add enum releation option type

  NR.

  I don't see any urgency here.


- Handling better supported channel binding types for SSL
  implementations

  RFC. Seems simple enough.


- Rewriting the test of pg_upgrade as a TAP test - take two~

  NR. Looks like it be doable to get this in, but depends a bit on
  interactions with Andrew Dunstan / buildfarm.


- btree_gin, add support for uuid, bool, name, bpchar and anyrange types

  NR. Definitely submitted late (2018-02-20). But also not a complicated
  patch.


- ICU as default collation provider

  NR.  This has been posted 2018-02-10, submitted 2018-02-26. It's a
  large patch. Therefore I don't think this is elegible for v11.
  Commented on thread.


- Advanced partition matching for partition-wise join

  NR.  While CF entry has been created late 2018-02-27, it has been in
  development for much longer, thread started 2017-08-21.  Apparently
  because it dependent on partitionwise join?

  Given the level of review and size of patch I've a bit of a hard time
  seing this getting into v11.


- Nepali Snowball dictionary

  NR. Referred to new snowball upstream. Will need sync with new code,
  which the CF entry doesn't yet do. Therefore I think this should be
  marked RF, as proposed on thread.


- Remove DSM_IMPL_NONE

  NR. Code removal, so we probably want this despite being submitted
  late.


- Transactions involving multiple postgres foreign servers

  NR. This is an *old* thread.

  This is large, not reviewed a lot (Robert started some serious
  reviewing in Feb, large changes ensued). I unfortunately don't see
  this in v11.


- ON CONFLICT DO UPDATE for partitioned tables

  NR. CF entry was created recently (2018-02-28), but is based on an
  earlier patch.

  This is relatively large and not reviewed much. I don't quite see it.


- kNN for SP-GiST

  NR. Current CF entry created 2018-02-28, but there was one previous
  entry that got one cycle of review 2017-03-09.

  This appears to be a large patch dropped on the eve of the current CF,
  despite some older ancestry.  I think we should just move it.


- SQL/JSON support in PostgreSQL

  This appears to be the older version of "SQL/JSON support in
  PostgreSQL", plus some further patches, which all also have new
  entries. I think we should close this entry, even the references patch
  seems a challenge, we certainly can't bite off even more. RWF?


- Foreign Key Arrays

  RFC. This is a fairly large, but not huge patch.  While it's marked
  RFC, I'm not quite sure it's gotten enough detailed review.


- Support to COMMENT ON DATABASE CURRENT_DATABASE

  NR.  Tom proposes to reject, and I can't fault him.


- Handling of the input data errors in COPY FROM

  NR.  I can't see this going anywhere, the current incarnation uses
  PG_TRY/CATCH around InputFunctionCall.


- Add NOWAIT option to VACUUM and ANALYZE

  RFC.  Looks pretty trivial.


- Boolean partition syntax

  WOA.  There seems to be pretty strong consensus that the current
  approach isn't the right one. I think this should be RWF. Proposed on
  thread.


- Lockable views

  RFC.  While marked as RFC, I think there's actually quite an
  uncertainty whether we want the feature in the current form at all.

  I think it'd be good for others to weigh in.


- generated columns

  NR.  This hasn't yet gotten much detailed review.  Not sure if
  realistic.


- MERGE

  NR.  There's qutie some review activity. Seems possible to get
  there. We'll have to see.


- SQL/JSON: functions

  NR.  Extracted from a bigger patch, submitted 2018-01-10.

  This has not gotten any review so far and is huge.  I don't see it.

  I think the SQL/JSON folks gotta prioritize. There's a numb

Re: Changing the autovacuum launcher scheduling; oldest table first algorithm

[Andres Freund]

Hi,

On 2018-02-28 18:04:27 +0900, Masahiko Sawada wrote:
> I've created the new thread for the changing AV launcher scheduling.
> The problem of AV launcher scheduling is described on [1] but I
> summarize it here.

This is a new patch submitted to CF 2018-03. As that's the last CF for
v11, and this patch isn't trivial, I propose to instead move this to the
next CF.

Greetings,

Andres Freund

Re: Boolean partitions syntax

[Amit Langote]

Horiguchi-san,

On 2018/02/05 18:17, Kyotaro HORIGUCHI wrote:
> At Mon, 29 Jan 2018 13:21:54 +0900, Amit Langote wrote:
>> Partition bound literals as captured gram.y don't have any type
>> information attached.  They're carried over in a A_Const to
>> transformPartitionBoundValue() and coerced to the target partition key
>> type there.  Note that each of the the partition bound literal datums
>> received from gram.y is castNode(A_Const)'d in parse_utilcmds.c.
> 
> eval_const_expressions is already running under
> transformPartitionBoundValue to resolve remaining coercion.  I
> suppose we can use AexprConst to restrict the syntax within
> appropriate variations.  Please find the attached patch.
> It allows the following syntax as a by-prodcut.
> 
> | create table c4 partition of t for values in (numeric(1,0) '5');
> 
> Parser accepts arbitrary defined types but it does no harm.
> 
> | create table c2 partition of t for values from (line '{0,1,0}') to (1);
> | ERROR:  specified value cannot be cast to type double precision for column 
> "a"
> 
> It rejects unacceptable functions but the message may look
> somewhat unfriendly.
> 
> | =# create table c1 partition of t for values in (random());
> | ERROR:  syntax error at or near ")"
> | LINE 1: create table c1 partition of t for values in (random());
> |  ^
> (marker is placed under the closing parenthesis of "random()")
> 
> | =# create table c1 partition of t for values in (random(0) 'x');
> | ERROR:  type "random" does not exist
> | LINE 1: create table c1 partition of t for values in (random(0) 'x')...
> (marker is placed under the first letter of the "random".)

I had tried the approach your patch takes and had noticed that the syntax
had stopped accepting negative values (a regression test fails due to that).

create table foo (a int) partition by list (a);
create table foo1 partition of foo for values in (-1);
ERROR:  syntax error at or near "-"
LINE 1: create table foo1 partition of foo for values in (-1);

I guess the following in gram.y leaves out negative numbers:

/*
 * Constants
 */
AexprConst: Iconst
{
$$ = makeIntConst($1, @1);
}
| FCONST
{
$$ = makeFloatConst($1, @1);
}

I had tried fixing that as well, but it didn't readily work.

Thanks,
Amit

Re: Online enabling of checksums

[Andres Freund]

On 2018-03-01 16:18:48 +0100, Magnus Hagander wrote:
> On Thu, Mar 1, 2018 at 9:04 AM, Andres Freund  wrote:
> 
> > On 2018-03-01 12:56:35 +0500, Andrey Borodin wrote:
> > > I've tried to rebase this patch to 10 and, despite minor rebase issues
> > (oids, bgw_type, changes to specscanner), patch works fine.
> > > Do we provide backporting for such features?
> >
> > Definitely not. With very rare exceptions (OS compatibility and the
> > like), features aren't backported.
> >
> 
> Yeah. And definitely not something that both changes the format of
> pg_control (by adding new possible values to the checksum field) *and* adds
> a new WAL record type...

And even more so, I'm not even sure it makes sense to try to get this
into v11. This is a medium-large complicated feature, submitted to the
last CF for v11.  That's pretty late.  Now, Magnus is a committer, but
nevertheless...

Greetings,

Andres Freund

Re: [HACKERS] SQL/JSON in PostgreSQL

[Andres Freund]

On 2018-03-02 10:31:34 +0300, Oleg Bartunov wrote:
> Right, we divided it to manageable pieces as Andrew suggested.

Please close the corresponding CF entry next time, if you do so. It's a
bit painful having to reconstruct such things out of numerous large
threads.

Greetings,

Andres Freund

Re: autovacuum: change priority of the vacuumed tables

[Andres Freund]

Hi,

On 2018-02-19 17:00:34 +0100, Tomas Vondra wrote:
> I have a hard time understanding how adding yet another autovacuum
> table-level knob makes the DBA's life any easier. Especially when the
> behavior is so unreliable and does not really guarantee when the
> high-priority table will be cleaned up ...

Based on the criticism voiced and a quick skim of the proposal, this CF
entry appears to still be in its design phase. In my opinion this thus
isn't v11 material, and should be marked as 'returned with feedback'?


Greetings,

Andres Freund

Re: [HACKERS] SQL/JSON in PostgreSQL

[Oleg Bartunov]

On 2 Mar 2018 09:44, "Andres Freund"  wrote:

Hi,

This patchset currently has multiple CF entries:
https://commitfest.postgresql.org/17/1063/

and then subordinate ones like
https://commitfest.postgresql.org/17/1471/
https://commitfest.postgresql.org/17/1472/
https://commitfest.postgresql.org/17/1473/

Thus I'm marking this entry as returned with feedback.


Right, we divided it to manageable pieces as Andrew suggested.


- Andres

Re: [HACKERS] Early locking option to parallel backup

[Andres Freund]

On 2018-01-15 12:12:26 -0500, Robert Haas wrote:
> On Thu, Jan 11, 2018 at 9:02 AM, Stephen Frost  wrote:
> > Parallel pg_dump is based on synchronized transactions though and we
> > have a bunch of checks in ImportSnapshot() because a pg_dump parallel
> > worker also can't really be quite the same as a normal backend.  Perhaps
> > we could add on more restrictions in ImportSnapshot() to match the
> > restrictions for parallel-mode workers?  If we think there's other users
> > of SET TRANSACTION SNAPSHOT then we might need to extend that command
> > for this case, but that seems relatively straight-forward.  I don't know
> > how reasonable the idea of taking a normally-started backend and making
> > it close enough to a parallel worker when a SET TRANSACTION SNAPSHOT
> > PARALLEL (or whatever) happens to allow it to skip the lock fairness is
> > though.
> 
> It seems pretty tricky to me.  I actually don't think this use case
> has all that much in common with the parallel query case.  Both can be
> addressed by tweaking the lock manager, but I think this needs a
> different set of tweaks than that did.

There seems to to be consensus in this thread that the approach Lucas
proposed isn't what we want, and that instead some shared lock based
approach is desirable.  As that has been the case for ~1.5 months, I
propose we mark this as returned with feedback?

Greetings,

Andres Freund

Re: Boolean partitions syntax

[Amit Langote]

On 2018/03/02 15:58, Andres Freund wrote:
> On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
>> Peter Eisentraut  writes:
>>> There might be other options, but one way to solve this would be to
>>> treat partition bounds as a general expression in the grammar and then
>>> check in post-parse analysis that it's a constant.
>>
>> That's pretty much what I said upthread.  What I basically don't like
>> about the current setup is that it's assuming that the bound item is
>> a bare literal.  Even disregarding future-extension issues, that's bad
>> because it can't result in an error message smarter than "syntax error"
>> when someone tries the rather natural thing of writing a more complicated
>> expression.
> 
> Given the current state of this patch, with a number of senior
> developers disagreeing with the design, and the last CF being in
> progress, I think we should mark this as returned with feedback.

I see no problem with pursuing this in the next CF if the consensus is
that we should fix how partition bounds are parsed, instead of adopting
one of the patches to allow the Boolean literals to be accepted as
partition bounds.

For the latter, my patch [1] would do the job, although after posting that
patch, the discussion turned into the one about the state of the current
partition bound parsing code.  I agree with most of the points raised and
Horiguchi-san even came up with a not-so-invasive patch to do that,
although, neither I, nor anyone else has been able to review or test it so
far.  I had written a patch like that one myself that I haven't shared on
the list, but had seen some problems with it.  I guess I should report
those in reply to Horiguchi-san's post.

That said, after seeing David Rowley's post earlier today [2], it seems
that we may need to consider this issue a bug rather than a new feature.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/6844d7f9-8219-a9ff-88f9-82c05fc90d70%40lab.ntt.co.jp

[2]
https://www.postgresql.org/message-id/CAKJS1f-BL%2Br5FXSejDu%3D%2BMAvzRARaawRnQ_ZFtbv_o6tha9NJw%40mail.gmail.com

Re: [HACKERS] log_destination=file

[Andres Freund]

Hi,

On 2018-01-20 14:51:12 +0200, Magnus Hagander wrote:
> Finally found myself back at this one, because I still think this is a
> problem we definitely need to adress (whether with this file or not).

This has an open CF entry for CF 2018-03. Given that there's still a lot
of uncertainty what we want, I don't think that's a reasonable target.
I would suggest moving it to 'returned with feedback'. Arguments
against?

- Andres

Re: chained transactions

[Andres Freund]

Hi,

On 2018-02-28 22:35:52 -0500, Peter Eisentraut wrote:
> This feature is meant to help manage transaction isolation in
> procedures.

This is a major new feature, submitted the evening before the last CF
for v11 starts. Therefore I think it should just be moved to the next
fest, it doesn't seems realistic to attempt to get this into v11.

Greetings,

Andres Freund

Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

[Andres Freund]

Hi,


On 2018-02-20 12:10:22 -0300, Matheus de Oliveira wrote:
> I attached a patch to add support for changing ON UPDATE/DELETE actions of
> a constraint using ALTER TABLE ... ALTER CONSTRAINT.

This patch has been submitted to the last commitfest for v11 and is not
a trivial patch. As we don't accept such patches this late, it should be
moved to the next fest.  Any arguments against?

Greetings,

Andres Freund

Re: pg_dump outputs invalid syntax for partitions with bool partkeys in pg10

[Amit Langote]

Hi David.

On 2018/03/02 12:41, David Rowley wrote:
> Quite simply:
> 
> d1=# create table bp (b bool) partition by list(b);
> CREATE TABLE
> d1=# create table bp_f partition of bp for values in('f');
> CREATE TABLE
> d1=# \q
> $ createdb d2
> $ pg_dump d1 | psql d2
> 
> ...
> 
> ERROR:  syntax error at or near "false"
> LINE 2: FOR VALUES IN (false);
>^
> ERROR:  relation "public.bp_f" does not exist
> ERROR:  relation "public.bp_f" does not exist
> invalid command \.
> 
> I understand there's a thread [1] which is discussing making this
> valid syntax, but on skimming it, there's no mention of any bugs.

Thanks for the report.

I guess we don't have a Boolean partitioned table in tests covered by
pg_dump or I'd have heard about this syntax glitch before Horiguchi-san
mentioned it a few months back [1].

> I'd thought about something like the attached patch (against master)
> to fix, but that leaves pg_dumps that have already been taken a bit
> out in the cold, so perhaps we need to think harder about allowing
> this syntax.

I agree about going ahead with adding the syntax support, but there are
some arguments against the design of the proposed patches or rather about
the original design of partition bound parsing itself.

> This was noticed by Jesper Pedersen in [2], but I've diagnosed this as
> an existing bug rather than a bug in the patch that thread relates to.

Certainly.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/20171128.203915.26713586.horiguchi.kyotaro%40lab.ntt.co.jp

Re: Boolean partitions syntax

[Andres Freund]

On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
> > There might be other options, but one way to solve this would be to
> > treat partition bounds as a general expression in the grammar and then
> > check in post-parse analysis that it's a constant.
> 
> That's pretty much what I said upthread.  What I basically don't like
> about the current setup is that it's assuming that the bound item is
> a bare literal.  Even disregarding future-extension issues, that's bad
> because it can't result in an error message smarter than "syntax error"
> when someone tries the rather natural thing of writing a more complicated
> expression.

Given the current state of this patch, with a number of senior
developers disagreeing with the design, and the last CF being in
progress, I think we should mark this as returned with feedback.


Greetings,

Andres Freund

psql tab completion for ALTER INDEX SET

[Masahiko Sawada]

Hi,

I found that tab completion for ALTER INDEX SET [tab] doesn't support
the reloptions of brin and gist. Attached patch adds "buffering",
"pages_per_range" and "autosummarize" options.

Regards,

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


tab_completion_for_alter_index_set.patch
Description: Binary data

Re: [HACKERS] SQL/JSON in PostgreSQL

[Andres Freund]

Hi,

This patchset currently has multiple CF entries:
https://commitfest.postgresql.org/17/1063/

and then subordinate ones like
https://commitfest.postgresql.org/17/1471/
https://commitfest.postgresql.org/17/1472/
https://commitfest.postgresql.org/17/1473/

Thus I'm marking this entry as returned with feedback.

- Andres

Re: [HACKERS] [PATCH] kNN for SP-GiST

[Andres Freund]

Hi,

On 2018-03-01 00:58:42 +0300, Nikita Glukhov wrote:
> Attached 3rd version of kNN for SP-GiST.

Given that this was submitted to the last v11 CF, after not being
developed for a year, I think it's unfortunately too late for v11. As we
should be concentrating on getting things into v11, I think it'd be
appropriate to move this to the next CF.

Greetings,

Andres Freund

Re: GSoC 2017: weekly progress reports (week 6)

[Andres Freund]

This appears to be a duplicate of https://commitfest.postgresql.org/17/1466/ - 
as the other one is older, I'm closing this one.

Re: [PROPOSAL] Nepali Snowball dictionary

[Andres Freund]

Hi,

On 2018-02-28 11:16:24 +0300, Arthur Zakirov wrote:
> I've created the commitfest entry https://commitfest.postgresql.org/17/1569/

What is that entry for, if I may ask?  We need to wait for them to merge
it, then sync the snowball code, including the nepali dictionary. This
doesn't realistically seem doable for this commitfest. Therefore I think
this should be marked as 'returned with feedback'.

Greetings,

Andres Freund

Re: [HACKERS] path toward faster partition pruning

[Amit Langote]

On 2018/03/01 21:56, Robert Haas wrote:
> On Tue, Feb 27, 2018 at 4:33 AM, Amit Langote
>  wrote:
>> Attached an updated version in which I incorporated some of the revisions
>> that David Rowley suggested to OR clauses handling (in partprune.c) that
>> he posted as a separate patch on the run-time pruning thread [1].
> 
> I'm very skeptical about this patch's desire to remove the static
> qualifier from evaluate_expr().  Why does this patch need that and
> constraint exclusion not need it?  Why should this patch not instead
> by using eval_const_expressions?  partkey_datum_from_expr() is
> prepared to give up if evaluate_expr() doesn't return a Const, but
> there's nothing in evaluate_expr() to make it give up if, for example,
> the input is -- or contains -- a volatile function, e.g. random().

Thinking on this a bit, I have removed the evaluate_expr() business from
partkey_datum_from_expr() and thus switched evaluate_expr() back to static.

Let me explain why I'd added there in the first place -- if the constant
expression received in partkey_datum_from_expr() was not of the same type
as that of the partition key, it'd try to coerce_to_target_type() the
input expression to the partition key type which may result in a non-Const
expression.  We'd turn it back into a Const by calling evaluate_expr().  I
thought the coercion was needed because we'd be comparing the resulting
datum with the partition bound datums using a partition comparison
function that would require its arguments to be of given types.

But I realized we don't need the coercion.  Earlier steps would have
determined that the clause from which the expression originated contains
an operator that is compatible with the partitioning operator family.  If
so, the type of the expression in question, even though different from the
partition key type, would be binary coercible with it.  So, it'd be okay
to pass the datum extracted from such expression to the partition
comparison function to compare it with datums in PartitionBoundInfo,
without performing any coercion.

> +   if (OidIsValid(get_default_oid_from_partdesc(partdesc)))
> +   rel->has_default_part = true;
> +   else
> +   rel->has_default_part = false;
>
> This can be written a lot more compactly as rel->has_default_part =
> OidIsValid(get_default_oid_from_partdesc(partdesc));

Indeed, will fix.

> PartitionPruneContext has no comment explaining its general purpose; I
> think it should.

Will fix.

Thanks,
Amit

Re: [HACKERS] Can ICU be used for a database's default sort order?

[Andres Freund]

Hi,

On 2018-02-10 20:45:40 +0500, Andrey Borodin wrote:
> I've contacted Postgres Professional. Marina Polyakova had kindly provided 
> their patch.
> The patch allows to use libc locale with ICU collation as default for cluster 
> or database.
> 
> It seems that this patch brings important long-awaited feature and deserves 
> to be included in last v11 commitfest.
> Peter, everyone, do you agree with this? Or should we better adapt this work 
> through v12 cycle?
> 
> I'm planning to provide review asap and do necessary changes if required 
> (this was discussed with Marina and Postgres Professional).

This patch was submitted for the last v11 commitfest, it's not a trivial
patch, and hasn't yet been reviewed. I'm afraid the policy is that large
patches shouldn't be submitted for the last commitfest...  Thus I think
this should be moved to the next one.

Greetings,

Andres Freund

Re: Rewriting the test of pg_upgrade as a TAP test - take two

[Andres Freund]

Hi,

On 2018-01-26 17:00:26 +0900, Michael Paquier wrote:
> Another topic that I would like to discuss is how this interface is fit
> for the buildfarm code.  After hacking my stuff, I have looked at the
> buildfarm code to notice that things like TestUpgradeXversion.pm do
> *not* make use of test.sh, and that what I hacked is much similar to
> what the buildfarm code is doing, which is itself roughly a copy of what
> test.sh does.  Andrew, your feedback would be important here.

Andrew, any comments?



> From 1294bb18426cea5c0764d0d07846fee291502b73 Mon Sep 17 00:00:00 2001
> From: Michael Paquier 
> Date: Wed, 24 Jan 2018 16:19:53 +0900
> Subject: [PATCH 1/3] Refactor PostgresNode.pm so as nodes can use custom
>  binary path
> 
> This is a requirement for having a proper TAP test suite for pg_upgrade
> where users have the possibility to manipulate nodes which use different
> set of binaries during the upgrade processes.

Seems reasonable.

> + # Find binary directory for this new node.
> + if (!defined($bindir))
> + {
> + # If caller has not defined the binary directory, find it
> + # from pg_config defined in this environment's PATH.
> + my ($stdout, $stderr);
> + my $result = IPC::Run::run [ 'pg_config', '--bindir' ], '>',
> + \$stdout, '2>', \$stderr
> + or die "could not execute pg_config";
> + chomp($stdout);
> + $bindir = $stdout;
> + }
> +

This isn't really required, we could just assume things are on PATH if
bindir isn't specified.  Don't have strong feelings about it.



> From df74a70b886bb998ac37195b4d3067c41a012738 Mon Sep 17 00:00:00 2001
> From: Michael Paquier 
> Date: Wed, 24 Jan 2018 16:22:00 +0900
> Subject: [PATCH 2/3] Add basic TAP test setup for pg_upgrade
> 
> The plan is to convert the current pg_upgrade test to the TAP
> framework.  This commit just puts a basic TAP test in place so that we
> can see how the build farm behaves, since the build farm client has some
> special knowledge of the pg_upgrade tests.

Not sure I see the point of keeping this separate, but whatever...


> From f903e01bbb65f1e38cd74b01ee99c4526e4c3b7f Mon Sep 17 00:00:00 2001
> From: Michael Paquier 
> Date: Fri, 26 Jan 2018 16:37:42 +0900
> Subject: [PATCH 3/3] Replace pg_upgrade's test.sh by a TAP test
> 
> This replacement still allows tests across major versions, though the
> interface to reach that has been changed a bit. See TESTING for more
> details on the matter.

> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> @@ -0,0 +1,213 @@
> +# Set of tests for pg_upgrade.
> +use strict;
> +use warnings;
> +use Cwd;
> +use Config;
> +use File::Basename;
> +use File::Copy;
> +use IPC::Run;
> +use PostgresNode;
> +use TestLib;
> +use Test::More tests => 4;
> +
> +# Generate a database with a name made of a range of ASCII characters.
> +sub generate_db
> +{

s/_/_random_/?


> +# From now on,

Odd phrasing imo.

>

> +elsif (defined($ENV{oldsrc}) &&
> + defined($ENV{oldbindir}) &&
> + defined($ENV{oldlibdir}))
> +{
> + # A run is wanted on an old version as base.
> + $oldsrc = $ENV{oldsrc};
> + $oldbindir = $ENV{oldbindir};
> + $oldlibdir = $ENV{oldlibdir};
> + # FIXME: this needs better tuning. Using "." or "$oldlibdir/postgresql"
> + # causes the regression tests to pass but pg_upgrade to fail afterwards.

Planning to fix it?


> +# Install manually regress.so, this got forgotten in the process.
> +copy "$oldsrc/src/test/regress/regress.so",
> + "$oldlibdir/postgresql/regress.so"
> + unless (-e "$oldlibdir/regress.so");

Weird comment again.


> +# Run regression tests on the old instance, using the binaries of this
> +# instance. At the same time create a tablespace path needed for the
> +# tests, similarly to what "make check" creates.

What does "using binaries of this instance" mean? And why?

> +chdir dirname($regress_bin);
> +rmdir "testtablespace";
> +mkdir "testtablespace";
> +$oldnode->run_log([ "$oldbindir/createdb", '--port', $oldnode->port,
> + 'regression' ]);

> +$oldnode->command_ok([$regress_bin, '--schedule', './serial_schedule',
> + '--dlpath', "$dlpath", '--bindir', $oldnode->bin_dir,
> + '--use-existing', '--port', $oldnode->port],
> + 'regression test run on old instance');


> +# Before dumping, get rid of objects not existing in later versions. This
> +# depends on the version of the old server used, and matters only if the
> +# old and new source paths
> +my $oldpgversion;
> +($result, $oldpgversion, $stderr) =
> + $oldnode->psql('postgres', qq[SHOW server_version_num;]);
> +my $fix_sql;
> +if ($newsrc ne $oldsrc)
> +{
> + if ($oldpgversion <= 80400)
> + {
> + $fix_sql = "DROP FUNCTION public.myfunc(integer); DROP FUNCTION 
> public.oldstyle_length(integer, text);";
> + }
> + else
> + {
> + $fix_sql =

Re: [WIP PATCH] Index scan offset optimisation using visibility map

[Andrey Borodin]

Hi, Michail!

Here are points that we need to address before advancing the patch.

> 20 февр. 2018 г., в 11:45, Andrey Borodin  написал(а):
> 
> Minor spots:
> There are some trailing whitespaces at line ends
>> Offset cannot be optimized because parallel execution
> I'd replace with 
>> Offset cannot be optimized [in spite of | due to] parallel execution
> 

> From my point of view, you should add to patch some words here
> https://www.postgresql.org/docs/current/static/indexes-index-only-scans.html

And few thoughts about plan cost estimation. Plz check create_limit_path() 
logic on cost estimation. I don't think you have enough information there to 
guess about possibility of IoS offset computation.
> I do not know if it is possible to take into account this optimization in 
> cost estimation.
> Does Limit node take cost of scanning into startup cost?

I'm marking patch as WoA, plz ping it to Need Review when done.

Thanks!

Best regards, Andrey Borodin.

Re: New gist vacuum.

[Andrey Borodin]

> 1 марта 2018 г., в 22:44, Tom Lane  написал(а):
> 
> Michail Nikolaev  writes:
>> I have added small change to patch to allow it be compiled using msvc 
>> (uint64_t -> uint64).
>> Everything seems to work, check-world is passing.
> 
>> Actually patch fixes two issues:
>> 1) Partial GIST indexes now have corrent tuples count estimation.
>> 2) Now subsequent calls to VACUUM on GIST index (like "VACCUM table_name") 
>> do not change tuples count to estimated number of tuples in table (which is 
>> changed even without any updates in table due current implementation).
> 
>> I think it is fine to commit.
> 
> I took a quick look at this.  I wonder what is the point of making
> the counting conditional.  Since the function is visiting every
> index page anyway, why not just always count and unconditionally
> provide an exact answer?  The number of cycles saved by skipping
> "tuplesCount += PageGetMaxOffsetNumber(page)" on each leaf page
> is surely trivial.

Thanks for looking into the patch, Tom!

I thought that it's a good idea to optimize out as many cycles as possible.
But, indeed, there are some reasons in favor of unconditional counting:
1. Code is cleaner, and this is not hot path
2. If we choose unconditional counting in gistvacuumcleanup() I'll remove those 
counting cycles in gistbulkdelete() in main vacuum patch (for v12). Both 
functions will have less code.

So, I agree, unconditional counting is a good idea. Here's the v3 patch.

Best regards, Andrey Borodin.


0001-Fix-GiST-stats-for-partial-indexes-v3.patch
Description: Binary data

Re: Failed to request an autovacuum work-item in silence

[Masahiko Sawada]

Thank you the comment.

On Fri, Mar 2, 2018 at 4:18 AM, Alvaro Herrera  wrote:
> Masahiko Sawada wrote:
>
>> While reading the code, I realized that the requesting an autovacuum
>> work-item could fail in silence if work-item array is full. So the
>> users cannot realize that work-item is never performed.
>> AutoVacuumRequestWork() seems to behave so from the initial
>> implementation but is there any reason of such behavior? It seems to
>> me that it can be a problem even now that there is only one kind of
>> work-item. Attached patch for fixing it.
>
> Hmm, yeah.
>
> I think it would be better to return false to caller, and have them
> report the failure.  Then they're in a better position to place an
> accurate log message -- for instance indicating the relation name rather
> than just OID, and specify work item type rather than some weird
> integer.  (I think the ereport() should definitely be *outside* the
> locked section, in any case.)

Agreed. Attached an updated patch.

>
> I'm inclined to change the API of AutoVacuumRequestWork even in branch
> 10.  Since this stuff is not nicely extensible (c.f. perform_work_item),
> it doesn't seem likely that any outside code is calling it yet.  Once we
> have hooks to register worker functions and stuff, it'll become more of
> a problem.

+1
Since this API cannot be execute actually other than brin
summarization I think we can change it in branch 10.

It's good if autovacuum work-items can be added by extensions and so
on. Also I'm inclined to change the autovacuum launcher so that it can
launcher new worker for performing work-item or to allow requests to
specify the execution timing (before or after vacuum), it's for PG12
item though.

Regards,

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


report_autovac_workitem_request_failure_v2.patch
Description: Binary data

Re: change in behaviour for format_type() call

[Rushabh Lathia]

On Thu, Mar 1, 2018 at 10:10 PM, Tom Lane  wrote:

> I wrote:
> > I don't see anything in the commit message or linked discussion to
> > indicate that any visible behavior change was intended, so I think
> > you're right, this is a bug.  Will check and push your patch.
>
> Actually, this patch still wasn't quite right: although it fixed
> one aspect of the behavior, it still produced identical results
> for typemod NULL and typemod -1, which as the function's comment
> explains is not what should happen.  I tweaked the logic to look
> as much as possible like before, and added a regression test.
>

Thanks Tom.


Regards,
Rushabh Lathia
www.EnterpriseDB.com

Re: [HACKERS][PATCH] Applying PMDK to WAL operations for persistent memory

[Yoshimi Ichiyanagi]

<20180301103641.tudam4mavba3g...@alap3.anarazel.de>
Thu, 1 Mar 2018 02:36:41 -0800Andres Freund  wrote :

Re: [HACKERS][PATCH] Applying PMDK to WAL operations for persistent 
memory
>On 2018-02-05 09:59:25 +0900, Yoshimi Ichiyanagi wrote:
>> I added my patches to the CommitFest 2018-3.
>> https://commitfest.postgresql.org/17/1485/
>
>Unfortunately this is the last CF for the v11 development cycle. This is
>a major project submitted late for v11, there's been no code level
>review, the goals aren't agreed upon yet, etc. So I'd unfortunately like
>to move this to the next CF?

I get it. I modified the status to "move to next CF".

-- 
Yoshimi Ichiyanagi
NTT laboratories

Re: PATCH: Unlogged tables re-initialization tests

[Michael Paquier]

On Thu, Mar 01, 2018 at 01:12:19PM -0500, David Steele wrote:
> But your point is well-taken.  No symlinks are used in this test so it
> *should* work.
>
> Michael, what do you think?

Perl's symlink() does not work on Windows.  It does not fly higher than
that, and that's the reason why a good chunk of the tests are skipped
for pg_basebackup.  If perl was to have a version of symlink() which
works, say with junction points, or if Windows was to have a sane
symlink implementation (or with [1]?), or if it was possible to create
junction points using an in-core implementation in perl, then those
tests could not be skipped. But it seems that none of those scenarios
have happened yet.

From what I read in your patch, it seems to me that this test should
work.  If they don't for whatever reason, your patch then does not give
a correct justification explaining why they should be skipped.

[1]: https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/
--
Michael


signature.asc
Description: PGP signature

Re: 2018-03 Commitfest Summary (Andres #3)

[Amit Langote]

Hi Andres.

On 2018/03/02 13:34, Andres Freund wrote:
> - reorganize partitioning code
> 
>   NR. Created recently, but split off an older patch.
> 
>   Seems like a generally reasonable idea. Wonder if it conflicts with
>   some other partition related patches?

It actually does.  There are at least a few other functionality patches
that touch same files and it may not be a good idea for them to have to
worry about conflicts with this.

I gave up on rebasing this patch yesterday as I couldn't finish it in 5
minutes, but maybe I will try later this month.  Gotta focus on the faster
pruning stuff for now...

> - Add support for tuple routing to foreign partitions
> 
>   NR. Patch is still evolving, the latest versions haven't gotten a lot
>   of review.
> 
>   It seems a bit ambitious to try to get this into v11.

I've looked at this a bit yesterday and plan to do review it sometime this
month.  Having skimmed it, the new FDW API added by the patch looks well
documented and seems to do the very specific job assigned to it well
enough, but then again I haven't done a full review yet.

Thanks,
Amit

Re: Add default role 'pg_access_server_files'

[Andres Freund]

Hi,

On 2018-01-19 09:28:33 -0500, Stephen Frost wrote:
> This patch still needs updating for Magnus' comments, of course, and
> I'm still planning to make that happen, so Waiting on Author is the
> right status currently.

Given that this hasn't happened, and that the next CF has started, ISTM,
the patch should be marked as returned with feedback. If not, it at the
very least should be updated soon...

Greetings,

Andres Freund

Re: TAP test module - PostgresClient

[Michael Paquier]

On Thu, Mar 01, 2018 at 02:27:13AM -0800, Andres Freund wrote:
> If I understand correctly there's been no progress on this since, and
> there'd definitely need to be major work to get something we can agree
> upon. Doesn't seem v11 material. I think we should mark this as returned
> with feedback.  Arguments against?

Agreed with your position.  The TAP tests rely on IPC::Run as a pillar
of its infrastructure.  I think that if we need a base API to do such
capabilities we ought to prioritize what we can do with it first instead
of trying to reinvent the wheel as this patch proposes in such a
complicated way.
--
Michael


signature.asc
Description: PGP signature

Re: 2018-03 Commitfest Summary (Andres #3)

[Andres Freund]

On 2018-03-01 14:45:15 -0800, Andres Freund wrote:
> Second round.

And last round. [ work ]. Scratch that. There'll be one more after this
;)


- Subscription code improvements

  WOA.  This is some minor cleanup stuff afaics. Don't think it's
  terribly release bound.


- Bootstrap data conversion

  NR.

  Not quite sure if it's realistic to do something about for v11. If so,
  it'd probably reasonable to merge this *after* the formal code freeze,
  to avoid breaking ~all pending patches.


- reorganize partitioning code

  NR. Created recently, but split off an older patch.

  Seems like a generally reasonable idea. Wonder if it conflicts with
  some other partition related patches?


- get rid of StdRdOptions, use individual binary reloptions
  representation for each relation kind instead

  NR. Created days ago, split off another patch.

  I'm unclear why this is particularly desirable, but I might just be
  missing something.


- Rewrite of pg_dump TAP tests

  WOA. Recently created.

  As it's "just" test infrastructure and makes things easier to
  understand, I think it makes sense to push this ASAP.


- remove pg_class.relhaspkey

  NR. Recentrly created.

  Minor cleanup, might break a few clients. Probably should do it, don't
  think it matters whether it's v11 or v12.


- pg_proc.prokind column

  RFC. Cleans up some sql procedure stuff.

  Obviously should get in.


- Unlogged tables re-initialization tests

  NR.

  Some questions about windows stuff that probably can easily be
  resolved within CF.


- fixing more format truncation issues

  NR.  Warnings cleanup / addition by configure.


- Make async slave to wait for lsn to be replayed

  WOA.

  There's been some design discussion a month ago, but no other
  activity. Given that this seems unlikely to be a candidate for
  v11. Should RWF.


- Logical decoding of two-phase transactions

  NR. Old thread, but lots of churn.

  I personally don't see how all of this patch series could get into
  v11. There's a bunch of fairly finnicky details here.


- Replication status in logical replication

  RFC.  Pretty small patch, Simon has claimed it.


- Restricting maximum keep segments by repslots

  NR.  Submitted a good while ago.

  I've some doubts about the design here, and there's certainly not been
  a lot of low level review.  Would need some reviewer focus to make it
  possible to get into v11.


- Exclude unlogged tables from base backups

  RFC, for quite a while.

  I still have some correctness doubts around this, will try to have a
  look.


- logical_work_mem limit for reorderbuffer
- logical streaming for large in-progress transactions

  NR.  I'm not quite sure why these in this CF, inquired on thread.


- TRUNCATE behavior when session_replication_role = replica

  RFC. Small patch, RFC since last CF. Should be merged.


- Logical decoding of TRUNCATE

  RFC.

  I think this isn't quite pretty, but I think that's largely because
  the DML vs DDL separation itself isn't really clean.


- Changing WAL Header to reduce contention during ReserveXLogInsertLocation()

  NR.

  We don't really seem to have agreement on what precisely to do, and
  there's no patch implementing the latest state of what's being
  discussed.  I'm not really certain why this in this CF.


- Checksums for slru files

  WOA.  Patch clearly not close to commit, and there's not much
  activity. Proposed to mark as RWF.


- handling of heap rewrites in logical decoding

  NR. Sent 2018-02-24, CFed 2018-02-28.

  This is an important feature, but I think it's going to have to wait
  for v12.


- Exclude temp relations from base backup

  NR. Sent 2018-02-28.

  But it's essentially a trivial extension from the other "exclude"
  patch above.  If we merge the above, we should just merge this as
  well.


- Verify Checksums during Basebackups

  NR. Sent 2018-02-28.

  It's a pretty simple patch, but it's been submitted on the day before
  the last CF. So I think there's a good case to be made for just moving
  it.


- GnuTLS support

  WOA.  I'm not quite sure what the status here is. If we want it, there
  seems to be some work to make it fully compatible, including scram
  work.


- Group Read Access for Data Directory

  WOA, although I don't quite know what for.

  Seems mostly ready, but I'm not sure how detailed a code review this
  got. A lot of the discussion in the thread is about tangential stuff.


- macOS Secure Transport SSL Support

  NR, CF entry created recently, but thread started much earlier.

  This is a fairly large part, and there seem to be some open
  issues. Being pre-auth and network exposed, It's also an area of code
  that is more security critical than a lot of other stuff. OTOH, there
  seems to be quite some desire to get off openssl on macs...


- pg_hba.conf : new auth option : clientcert=verify-full

  NR. Submitted first 2018-02-16.

  Fairly trivial patch, but submitted to last CF.  I think this is
  pretty borderline.


- Cor

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

On Thu, Mar 01, 2018 at 02:28:54AM -0800, Andres Freund wrote:
> On 2018-02-01 09:04:57 -0500, Peter Eisentraut wrote:
>> I've been testing this a bit further and during a test setup with 4-byte
>> bools I still got regression test failures related to GIN, so it doesn't
>> seem quite ready.  I'll keep working on it.
> 
> Are you planning to get this into v11? The CF entry
> https://commitfest.postgresql.org/17/1444/
> lists this as "ready for committer" which doesn't seem to jive with the
> above description?

Indeed, this is wrong.  Peter, why did you switch suddendly this patch
as ready for committer?  The patch is waiting for your input as you
mentioned that the GIN portion of this patch series is not completely
baked yet.  So I have switched the patch in this state.
--
Michael


signature.asc
Description: PGP signature

Re: [PROPOSAL] Shared Ispell dictionaries

[Andres Freund]

Hi,

On 2018-02-07 19:28:29 +0300, Arthur Zakirov wrote:
> + {
> + {"max_shared_dictionaries_size", PGC_POSTMASTER, RESOURCES_MEM,
> + gettext_noop("Sets the maximum size of all text search 
> dictionaries loaded into shared memory."),
> + gettext_noop("Currently controls only loading of Ispell 
> dictionaries. "
> +  "If total size of 
> simultaneously loaded dictionaries "
> +  "reaches the maximum allowed 
> size then a new dictionary "
> +  "will be loaded into local 
> memory of a backend."),
> + GUC_UNIT_KB,
> + },
> + &max_shared_dictionaries_size,
> + 100 * 1024, 0, MAX_KILOBYTES,
> + NULL, NULL, NULL
> + },

So this uses shared memory, allocated at server start?  That doesn't
seem right. Wouldn't it make more sense to have a
'num_shared_dictionaries' GUC, and then allocate them with dsm? Or even
better not have any such limit and us a dshash table to point to
individual loaded tables?

Is there any chance we can instead can convert dictionaries into a form
we can just mmap() into memory?  That'd scale a lot higher and more
dynamicallly?

Regards,

Andres

Re: [HACKERS] logical decoding of two-phase transactions

[Nikhil Sontakke]

Hi Andres and Craig,

>> In the future you should number them. Right now they appear to be out of
>> order in your email.  I suggest using git format-patch, that does all
>> the necessary work for you.
>>
> Yep, specially git format-patch with a -v argument, so the whole patchset is
> visibly versioned and sorts in the correct order.
>

I did try to use *_Number.patch to convey the sequence, but admittedly
it's pretty lame.

I will re-submit with "git format-patch" soon.

Regards,
Nikhils
-- 
 Nikhil Sontakke   http://www.2ndQuadrant.com/
 PostgreSQL/Postgres-XL Development, 24x7 Support, Training & Services

Re: PATCH: Configurable file mode mask

[Michael Paquier]

On Thu, Mar 01, 2018 at 09:32:58PM -0500, David Steele wrote:
> On 3/1/18 9:04 PM, Andres Freund wrote:
>> I'd suggest just using git format-patch -v to format series.
> 
> Sure, I've been meaning to switch over to this format and just haven't
> gotten around to it yet.  I do recognize the value, however, and will do it
> going forward.

Simple patches can bypass on that, but once you have at least three
patches this helps a lot with reviews.  Explaining as well in the email
sending the patches what each patch actually does, even roughly helps in
focusing on one element or the other.
 
>> E.g. here it's far from obvious why something is done with
>> pg_resetwal.
> [reordered by me for clarity]
> 
> Any front-end tool that writes into PGDATA is affected by this patch because
> mode must be set correctly.
> 
> pg_resetwal had no tests, so I wrote a basic test suite to base the group
> permissions tests on.  I included the basic test suite as a separate patch
> because I felt it should be committed even if the main feature wasn't.  It's
> far from a comprehensive test suite, but certainly better than what we have
> currently.

Based on my recent lookup at code level for this feature, the patch for
pg_resetwal (which could have been discussed on its own thread as well),
would be fine for commit.  The thing could be extended a bit more but
there is nothing opposing even a basic test suite to be in.  Then you
have a set of refactoring patches, which still need some work.  And
finally there is a rather invasive patch on top of the whole thing.  The
refactoring work shows much more value only after the main feature is
in, still I think that unifying the default permissions allowed for
files and directories, as well as mkdir() calls has some value in
itself to think it as an mergeable, independent, change.  I think that
it would be hard to get the whole patch set into the tree by the end of
the CF though, but cutting the refactoring pieces would be doable.  At
least it would provide some base for integration in v12.  And the
refactoring patch has some pieces that would be helpful for TAP tests as
well.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Creating backup history files for backups taken from standbys

[David Steele]

On 3/1/18 11:07 PM, Michael Paquier wrote:
> On Fri, Mar 02, 2018 at 02:29:13AM +0900, Fujii Masao wrote:
>> + * write a backup history file with the same name.
>>
>> So more than one backup history files with the same name
>> but the diffferent content can be created and archived.
>> Isn't this problematic because the backup history file that
>> users want to use later might be overwritten unexpectedly?
> 
> Yeah, that's the intention behind the patch.  Would that actually happen
> in practice though?  We would talk about two backups running
> simultaneously on a standby, which would overlap with each other to
> generate a file aimed only at being helpful for debugging purposes, and
> we provide no information now for backups taken from standbys.  We could
> of course make that logic a bit smarter by checking if there is an
> extsing file with the same name and create a new file with a different
> name.  But is that worth the complication? That's where I am not
> convinced, and that's the reason why this patch is doing things this
> way.

+1.  Given that the history files are not used during restore and are
present primarily for debugging purposes, I can't see worrying too much
about this unlikely (if possible) race condition.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: RFC: Add 'taint' field to pg_control.

[Justin Pryzby]

On Thu, Mar 01, 2018 at 09:12:18AM +0800, Craig Ringer wrote:
> On 1 March 2018 at 06:28, Justin Pryzby  wrote:
> > The more fine grained these are the more useful they can be:
> >
> > Running with fsync=off is common advice while loading, so reporting that
> > "fsync=off at some point" is much less useful than reporting "having to run
> > recovery from an fsync=off database".
> >
> > I propose there could be two bits:
> >
> >  1. fsync was off at some point in history when the DB needed recovery;
> >  2. fsync was off during the current instance; I assume the bit would be set
> > whenever fsync=off was set, but could be cleared when the server was
> > cleanly shut down.  If the bit is set during recovery, the first bit 
> > would
> > be permanently set.  Not sure but if this bit is set and then fsync
> > re-enabled, maybe this could be cleared after any checkpoint and not 
> > just
> > shutdown ?
> >
> I think that's a very useful way to eliminate false positives and make this
> diagnostic field more useful. But we'd need to guarantee that when you've
> switched from fsync=off to fsync=on, we've actually fsync'd every extent of
> every table and index, whether or not we think they're currently dirty and
> whether or not the smgr currently has them open. Do something like initdb
> does to flush everything. Without that, we could have WAL-vs-heap ordering
> issues and so on *even after a Pg restart* if the system has lots of dirty
> writeback to do.


I think a similar, "2 bit" scheme *could* be applied to full_page_writes, too.
And the 2nd "soft" bit for FPW could also be cleared after checkpoint.  I'd
think that for FPW, previous checkpoints which had fsync=off wouldn't need
special handling - so long as FPW=on, the "soft" bit can be cleared after
successful checkpoint (as a micro-optimization, don't clear the "soft" bit if
the "hard" bit is set).

BTW this scheme has the amusing consequence that setting fsync=off should
involve first writing+fsyncing these bits to pgcontrol.

Justin

Re: [HACKERS] Statement-level rollback

[Andres Freund]

Hi,

On 2018-01-09 08:21:33 +, Tsunakawa, Takayuki wrote:
> From: Simon Riggs [mailto:si...@2ndquadrant.com]
> > When will the next version be posted?
> 
> I'm very sorry I haven't submitted anything.  I'd like to address this during 
> this CF.  Thanks for remembering this.

Given that no new version has been submitted since, that this is the
last CF, and that we are far from agreeing on a design, I'm marking this
as returned with feedback.

Greetings,

Andres Freund

Re: [HACKERS] Creating backup history files for backups taken from standbys

[Michael Paquier]

On Thu, Mar 01, 2018 at 07:26:09PM -0800, Andres Freund wrote:
> On 2018-03-02 02:29:13 +0900, Fujii Masao wrote:
>> + * write a backup history file with the same name.
>> 
>> So more than one backup history files with the same name
>> but the diffferent content can be created and archived.
>> Isn't this problematic because the backup history file that
>> users want to use later might be overwritten unexpectedly?
> 
> Michael?

Just answered to that question.
--
Michael


signature.asc
Description: PGP signature

Re: WIP: BRIN multi-range indexes

[Tom Lane]

Andres Freund  writes:
> On 2018-02-25 01:30:47 +0100, Tomas Vondra wrote:
>> Note: Currently, this only works with float8-based data types.
>> Supporting additional data types is not a big issue, but will
>> require extending the opclass with "subtract" operator (used to
>> compute distance between values when merging ranges).

> Based on Tom's past stances I'm a bit doubtful he'd be happy with such a
> restriction.  Note that something similar-ish also has come up in
> 0a459cec96.

> I kinda wonder if there's any way to not have two similar but not equal
> types of logic here?

Hm.  I wonder what the patch intends to do with subtraction overflow,
or infinities, or NaNs.  Just as with the RANGE patch, it does not
seem to me that failure is really an acceptable option.  Indexes are
supposed to be able to index whatever the column datatype can store.

regards, tom lane

Re: [HACKERS] Creating backup history files for backups taken from standbys

[Michael Paquier]

On Fri, Mar 02, 2018 at 02:29:13AM +0900, Fujii Masao wrote:
> + * write a backup history file with the same name.
> 
> So more than one backup history files with the same name
> but the diffferent content can be created and archived.
> Isn't this problematic because the backup history file that
> users want to use later might be overwritten unexpectedly?

Yeah, that's the intention behind the patch.  Would that actually happen
in practice though?  We would talk about two backups running
simultaneously on a standby, which would overlap with each other to
generate a file aimed only at being helpful for debugging purposes, and
we provide no information now for backups taken from standbys.  We could
of course make that logic a bit smarter by checking if there is an
extsing file with the same name and create a new file with a different
name.  But is that worth the complication? That's where I am not
convinced, and that's the reason why this patch is doing things this
way.
--
Michael


signature.asc
Description: PGP signature

Re: change in behaviour for format_type() call

[Michael Paquier]

On Thu, Mar 01, 2018 at 02:54:22PM -0300, Alvaro Herrera wrote:
> Tom Lane wrote:
>> Actually, this patch still wasn't quite right: although it fixed
>> one aspect of the behavior, it still produced identical results
>> for typemod NULL and typemod -1, which as the function's comment
>> explains is not what should happen.  I tweaked the logic to look
>> as much as possible like before, and added a regression test.
> 
> Thanks!

Thanks Tom.  Yes, we focused on not changing any existing behavior with
this patch.  So this report was a bug.
--
Michael


signature.asc
Description: PGP signature

Re: zheap: a new storage format for PostgreSQL

[Mark Kirkwood]

On 02/03/18 16:53, Alvaro Herrera wrote:


I think it was impolite to post this on the very same day the commitfest
started.  We have enough patches as it is ...



To be fair - he did say things like "wanting feedback..." and "shows an 
example of using pluggable storage.." and for PG 12. If he held onto the 
patches and waited - he'd get criticism of the form "you should have 
given a heads up earlier...".


This is earlier :-)

Best wishes

Mark

P.s: awesome work.

Re: [HACKERS] GSoC 2017: weekly progress reports (week 4) and patch for hash index

[Thomas Munro]

On Fri, Mar 2, 2018 at 3:57 PM, Andres Freund  wrote:
> Based on this sub-thread this patch's status of 'needs review' doesn't
> quite seem accurate and 'waiting on author' and then 'returned with
> feedback' would be more fitting?

I personally think this patch is really close to RFC.  Shubham has
fulfilled the project requirement, it's a tidy and short patch, it has
tests.  I think we really just need to verify that the split case
works correctly.

Hmm.  I notice that this calls PredicateLockPageSplit() after both
calls to _hash_splitbucket() (the one in _hash_finish_split() and the
one in _hash_expandtable()) instead of doing it inside that function,
and that _hash_splitbucket() unlocks bucket_nbuf before returning.
What if someone else accesses bucket_nbuf between
LockBuffer(bucket_nbuf, BUFFER_LOCK_UNLOCK) and
PredicateLockPageSplit()?  Doesn't that mean that another session can
read a newly created page and miss a predicate lock that is about to
be transferred to it?  If that is indeed a race, could it be fixed by
calling PredicateLockPageSplit() at the start of _hash_splitbucket()
instead?

Could we get a few days to mull over this and Shubham's other patches?
 It'd be really great to get some of these into 11.

My thought experiments about pseudo-pages and avoiding the split stuff
were not intended to get the patch kicked out.  I thought for a while
that hash indexes were a special case and could benefit from
dispensing with those trickier problems.  Upon further reflection, for
interesting size hash indexes pure hash value predicate tags wouldn't
be much better.  Furthermore, if we do decide we want to use using x %
max_predicate_locks_per_relation to avoid having to escalate to
relation predicate locks at the cost of slightly higher collision rate
then we should consider that for the whole system (including heap page
predicate locking), not just hash indexes.  Please consider those
ideas parked for now.

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

Re: WIP: BRIN multi-range indexes

[Andres Freund]

Hi,

On 2018-02-25 01:30:47 +0100, Tomas Vondra wrote:
> Note: Currently, this only works with float8-based data types.
> Supporting additional data types is not a big issue, but will
> require extending the opclass with "subtract" operator (used to
> compute distance between values when merging ranges).

Based on Tom's past stances I'm a bit doubtful he'd be happy with such a
restriction.  Note that something similar-ish also has come up in
0a459cec96.

I kinda wonder if there's any way to not have two similar but not equal
types of logic here?

That problem is
resolved here by adding the ability for btree operator classes to provide
an "in_range" support function that defines how to add or subtract the
RANGE offset value.  Factoring it this way also allows the operator class
to avoid overflow problems near the ends of the datatype's range, if it
wishes to expend effort on that.  (In the committed patch, the integer
opclasses handle that issue, but it did not seem worth the trouble to
avoid overflow failures for datetime types.)


- Andres

Re: zheap: a new storage format for PostgreSQL

[Alvaro Herrera]

I think it was impolite to post this on the very same day the commitfest
started.  We have enough patches as it is ...

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

pg_dump outputs invalid syntax for partitions with bool partkeys in pg10

[David Rowley]

Quite simply:

d1=# create table bp (b bool) partition by list(b);
CREATE TABLE
d1=# create table bp_f partition of bp for values in('f');
CREATE TABLE
d1=# \q
$ createdb d2
$ pg_dump d1 | psql d2

...

ERROR:  syntax error at or near "false"
LINE 2: FOR VALUES IN (false);
   ^
ERROR:  relation "public.bp_f" does not exist
ERROR:  relation "public.bp_f" does not exist
invalid command \.

I understand there's a thread [1] which is discussing making this
valid syntax, but on skimming it, there's no mention of any bugs.

I'd thought about something like the attached patch (against master)
to fix, but that leaves pg_dumps that have already been taken a bit
out in the cold, so perhaps we need to think harder about allowing
this syntax.

Thoughts?

This was noticed by Jesper Pedersen in [2], but I've diagnosed this as
an existing bug rather than a bug in the patch that thread relates to.

[1] 
https://www.postgresql.org/message-id/flat/e05c5162-1103-7e37-d1ab-6de3e0afa...@lab.ntt.co.jp#e05c5162-1103-7e37-d1ab-6de3e0afa...@lab.ntt.co.jp

[2] 
https://www.postgresql.org/message-id/c3eb3284-24c8-eff0-f930-a33984d11...@redhat.com

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


very_minimal_bool_partition_syntax_fix.patch
Description: Binary data

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Andres Freund]

Hi,

On 2018-02-24 23:01:59 +0100, Tomas Vondra wrote:
> Sadly, this patch series does not seem to move forward very much, and
> I'm not sure how to change that :-/

What's your estimate about the patchset's maturity?

Greetings,

Andres Freund

Re: [HACKERS] Creating backup history files for backups taken from standbys

[Andres Freund]

Hi!

On 2018-03-02 02:29:13 +0900, Fujii Masao wrote:
> On Fri, Feb 2, 2018 at 2:06 PM, Michael Paquier
>  wrote:
> > On Fri, Feb 02, 2018 at 12:47:26AM +0900, Fujii Masao wrote:
> >> The patch basically looks good to me. Here are some small comments.
> >>
> >>
> >>   The backup history file is not created in the database cluster 
> >> backed up.
> >>
> >>
> >> The above should be deleted in pg_basebackup.sgml.
> >>
> >> * During recovery, since we don't use the end-of-backup WAL
> >> * record and don't write the backup history file, the
> >>
> >> This comment needs to be updated in xlog.c.
> >
> > Thanks Fujii-san for the review.  Indeed those portions need a refresh..
> 
> Thanks for updating the patch!

You'd claimed this patch as a committer. You're still planning to commit
it?


> + * write a backup history file with the same name.
> 
> So more than one backup history files with the same name
> but the diffferent content can be created and archived.
> Isn't this problematic because the backup history file that
> users want to use later might be overwritten unexpectedly?

Michael?

Greetings,

Andres Freund

Re: new function for tsquery creartion

[Andres Freund]

On 2017-11-29 17:56:30 +0300, Victor Drobny wrote:
> Thank you for review. I have tried to fix all of your comments.
> However i want to mention that the absence of comments for functions
> in to_tsany.c is justified by the absence of comments for other
> similar functions.

That's not justification. Tsquery related code is notorious for being
badly commented, we do not want to continue that.

Greetings,

Andres Freund

Re: [HACKERS] GSoC 2017: weekly progress reports (week 4) and patch for hash index

[Andres Freund]

Hi,

Based on this sub-thread this patch's status of 'needs review' doesn't
quite seem accurate and 'waiting on author' and then 'returned with
feedback' would be more fitting?

Greetings,

Andres Freund

Re: [HACKERS] [PATCH] Improve geometric types

[Andres Freund]

Hi,

On 2018-02-07 16:46:38 +0100, Emre Hasegeli wrote:
> I am incorporating the fix you have posted to the other thread to this patch.

You've not posted a version fixing the issues in the surrounding thread,
do I see that right?

I'm a bit confused how this patch has gone through several revisions
since, but has been marked as "ready for committer" since 2017-09-05. Am
I missing something?

Greetings,

Andres Freund

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

[Pavel Stehule]

2018-03-02 3:38 GMT+01:00 Andres Freund :

> On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
> > 2018-03-01 23:10 GMT+01:00 Andres Freund :
> >
> > > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > > > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > > > > This really could use a new thread, imv.  This thread is a year
> old and
> > > > > about a completely different feature than what you've implemented
> here.
> > > > >
> > > >
> > > > true, but now it is too late
> > >
> > > At the very least the CF entry could be renamed moved out the procedual
> > > language category?
> > >
> >
> > Why not?
>
> Because the patch adds GUCs that don't have a direct connection
> toprocedual languages?  And the patch's topic still says "plpgsql plan
> cache behave" which surely is misleading.
>
> Seems fairly obvious that neither category nor name is particularly
> descriptive of the current state?
>
>
ok


> > Have you idea, what category is best?
>
> Server Features? Misc?  And as a title something about "add GUCs to
> control custom plan logic"?
>
>
I'll move it there.

Regards

Pavel

>
> Greetings,
>
> Andres Freund
>

Re: zheap: a new storage format for PostgreSQL

[Amit Kapila]

On Fri, Mar 2, 2018 at 2:42 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Thu, Mar 1, 2018 at 5:09 PM, Amit Kapila 
> wrote:
>
>> Preliminary performance results
>> ---
>>
>> *We’ve shown the performance improvement of zheap over heap in a few
>> different pgbench scenarios.  All of these tests were run with data that
>> fits in shared_buffers (32GB), and 16 transaction slots per zheap page.
>> Scenario-1 and Scenario-2 has used synchronous_commit = off and Scenario-3
>> and Scenario-4 has used synchronous_commit = on*
>>
>
> What hardware did you use for benchmarks?
>
Also, I note that you have 4 transaction slots per zheap page in github
> code while you use 16 in benchmarks.
>
> #define MAX_PAGE_TRANS_INFO_SLOTS 4
>
> I would also note that in the code you preserve only 3 bits for
> transaction slot number.  So, one have to redefine 3 macros to change
> transaction slot number to the value you used in the benchmarks.
>
> #define ZHEAP_XACT_SLOT 0x3800 /* 3 bits (12, 13 and 14) for transaction
> slot */
> #define ZHEAP_XACT_SLOT_MASK 0x000B /* 11 - mask to retrieve transaction
> slot */
>
> I'm only starting reviewing this, but it makes me think that we need
> transaction slots number to be tunable (or even auto-tunable).
>
>
Yeah, that is the plan.  So, the idea is that for now we will give compile
time option to configure the number of slots (the patch for the same is
ready, currently we are testing it), later we can even give the option to
user at relation level or whatever we decides.  Why I think it makes sense
to give an option at relation level is that for larger relations, we can do
with very few transaction slots considering that the chances of many
transactions operating on the same page are less, it is only for smaller
relations that we need more number of slots.  OTOH, there could be
workloads where we can expect many concurrent transactions on the same
page.  However, for now if you want to test, the patch to increase
transaction slots is attached, you need to change the value of few macros
according to the number of slots you want.


> BTW, last two macros don't look properly named for me.  I would rather
> rename them in a following way:
> ZHEAP_XACT_SLOT_MASK => ZHEAP_XACT_SLOT_OFFSET
>

How about ZHEAP_XACT_SLOT_SHIFT?  I see similar things named with *_SHIFT
suffix in code .


> ZHEAP_XACT_SLOT => ZHEAP_XACT_SLOT_MASK
>
>
makes sense.  I will change it.


>
>> *Scenario 1: A 15 minutes simple-update pgbench test with scale factor
>> 100 shows 5.13% TPS improvement with 64 clients. The performance
>> improvement increases as we increase the scale factor; at scale factor
>> 1000, it reaches11.5% with 64 clients.Scale FactorHEAPZHEAP
>> (tables)*ImprovementBefore test1001281 MB1149 MB-10.3%100013 GB11
>> GB-15.38%After test1004.08 GB3 GB-26.47%100015 GB12.6 GB-16%* The size of
>> zheap tables increase because of the insertions in pgbench_history table.*
>>
>
> I think results representation should be improved.  You show total size of
> the database, but it's hard to understand how bloat degree was really
> decreased, assuming that there are both update and append-only tables.  So,
> I propose to show the results in per table manner.
>
>
Fair enough, Kuntal has done this testing.  He will share the results as
you have requested.


> What is total number of transactions processed in both cases?  It would be
> also more fair to compare sizes for the same number of processed
> transactions.
>
> Also, what are index sizes?  What are undo log sizes for zheap?
>
>

There shouldn't be any change in the index sizes and by the end of tests
undo is completely discarded.  I think to see the impact of undo size, we
need some different tests where in we can keep the transaction open till
end of test or some such.


> I also suggest to use Zipfian distribution in testing.  It's more close to
> real world workloads.  And it would be a good stress test for both HOT and
> transaction slots.
>
>
Yeah, we can do such tests, but keep in mid this code is still a work in
progress and lot of things are going to change and till now we have not
done much optimization in the code to improve the performance numbers.

Thanks a lot for showing interest in this work!

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


increase_slots_15.patch
Description: Binary data

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[David Steele]

Hi Tomas.

On 3/1/18 9:33 PM, Tomas Vondra wrote:

On 03/02/2018 02:12 AM, Andres Freund wrote:

On 2018-02-01 23:51:55 +0100, Tomas Vondra wrote:

On 02/01/2018 03:51 PM, Peter Eisentraut wrote:

To close out this commit fest, I'm setting both of these patches as
returned with feedback, as there are apparently significant issues to be
addressed.  Feel free to move them to the next commit fest when you
think they are ready to be continued.



Will do. Thanks for the feedback.


Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
but I don't see a newer version posted?



Ah, apologies - that's due to moving the patch from the last CF (it was
marked as RWF so I had to reopen it before moving it). I'll submit a new
version of the patch shortly, please mark it as WOA until then.


Marked as Waiting on Author.

--
-David
da...@pgmasters.net

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

[Andres Freund]

On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
> 2018-03-01 23:10 GMT+01:00 Andres Freund :
> 
> > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > > > This really could use a new thread, imv.  This thread is a year old and
> > > > about a completely different feature than what you've implemented here.
> > > >
> > >
> > > true, but now it is too late
> >
> > At the very least the CF entry could be renamed moved out the procedual
> > language category?
> >
> 
> Why not?

Because the patch adds GUCs that don't have a direct connection
toprocedual languages?  And the patch's topic still says "plpgsql plan
cache behave" which surely is misleading.

Seems fairly obvious that neither category nor name is particularly
descriptive of the current state?


> Have you idea, what category is best?

Server Features? Misc?  And as a title something about "add GUCs to
control custom plan logic"?


Greetings,

Andres Freund

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Tomas Vondra]

On 03/02/2018 02:12 AM, Andres Freund wrote:
> On 2018-02-01 23:51:55 +0100, Tomas Vondra wrote:
>> On 02/01/2018 03:51 PM, Peter Eisentraut wrote:
>>> To close out this commit fest, I'm setting both of these patches as
>>> returned with feedback, as there are apparently significant issues to be
>>> addressed.  Feel free to move them to the next commit fest when you
>>> think they are ready to be continued.
>>>
>>
>> Will do. Thanks for the feedback.
> 
> Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
> but I don't see a newer version posted?
> 

Ah, apologies - that's due to moving the patch from the last CF (it was
marked as RWF so I had to reopen it before moving it). I'll submit a new
version of the patch shortly, please mark it as WOA until then.

regards

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

Re: PATCH: Configurable file mode mask

[David Steele]

Hi Andres,

On 3/1/18 9:04 PM, Andres Freund wrote:

On 2018-02-27 15:52:32 -0500, David Steele wrote:

Thanks for having a look at the patches.


I'd personally appreciate if you'd add commit messages to the individual
patches in a series. A brief explanation why something is done is good
enough.

I'd suggest just using git format-patch -v to format series.


Sure, I've been meaning to switch over to this format and just haven't 
gotten around to it yet.  I do recognize the value, however, and will do 
it going forward.



Independent of that, I'm not entirely clear on what the status of this
patch is, without rereading the thread. Could you summarize?


Good question.  This patch has been around for a year and has gone 
though a number of iterations.


In summary, we started with a more rigid design that required a GUC to 
enable group privs (actually, at first it was any mode you wanted). 
Through feedback from Tom and others we realized that the front-end 
tools could not read the GUCs and it would probably be best based on the 
perms of PGDATA, as long as they were 700 or 750.


We decided the patch was too big, so broke it up a bit and got some of 
the infrastructure committed in 2017-09 [1].


In 2018-01 we brought in a unified patch set that built on the ideas 
from 2017-03.  During that CF we taught all the front-end tools to check 
PGDATA for permissions and wrote a lot of tests.


The current incarnation is a refinement with input from Michael and a 
different split in the patches.


> E.g. here it's far from obvious why something is done with
> pg_resetwal.
[reordered by me for clarity]

Any front-end tool that writes into PGDATA is affected by this patch 
because mode must be set correctly.


pg_resetwal had no tests, so I wrote a basic test suite to base the 
group permissions tests on.  I included the basic test suite as a 
separate patch because I felt it should be committed even if the main 
feature wasn't.  It's far from a comprehensive test suite, but certainly 
better than what we have currently.


Does that clear things up?

--
-David
da...@pgmasters.net

[1] https://commitfest.postgresql.org/14/1262/

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

[Daniel Gustafsson]

> On 02 Mar 2018, at 10:10, Andres Freund  wrote:
> 
> On 2018-01-26 23:30:08 +0100, Daniel Gustafsson wrote:
>> And another rebase and update after the refactoring in c1869542b3a4da4b12ca.
>> Also fixed some typos in comments.  The other patches originally posted in 
>> this
>> patchset are either committed or made redundant.
> 
> Could you provide a quick summary about where you think this patch
> stands currently? The cover letter you had for this thread was great…

I’m actually working on that right now, combined with a rebase.  I’m travelling
today and tomorrow with limited time and internet available but I will provide
this on Sunday at the latest.

cheers ./daniel

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

[Pavel Stehule]

2018-03-01 23:10 GMT+01:00 Andres Freund :

> On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > 2018-01-22 23:15 GMT+01:00 Stephen Frost :
> > > This really could use a new thread, imv.  This thread is a year old and
> > > about a completely different feature than what you've implemented here.
> > >
> >
> > true, but now it is too late
>
> At the very least the CF entry could be renamed moved out the procedual
> language category?
>

Why not? Have you idea, what category is best?

Regards

Pavel

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 2 March 2018 at 08:13, Robert Haas  wrote:
> I don't like the comments at the top of partprune.c very much.  It
> seems strange to document individual functions here; those functions
> can (and should) be documented in their individual header comments.
> What we should have at the top of the file is a discussion of the
> overall theory of operation of this module, something that currently
> seems not to exist anywhere in the patch.  I tried to figure that out
> by looking at the new data structures the patch introduces:
> PartitionPruneContext, PartScanKeyInfo, PartitionClauseInfo, and
> PartClause.  It looks like the general idea idea is that code that
> wants to use these facilities does the following:
>
> Step 1. Generate a PartitionPruneContext.  In this patch, this seems
> to consist entirely of copying information from the RelOptInfo or its
> PartitionScheme.
> Step 2. Call generate_partition_clauses() to extract relevant clauses
> from rel->baserestrictinfo and generate a PartClauseInfo.
> Step 3. Call get_partitions_from_clauses() to generate a set of
> unpruned partition indexes.  Internally, that function will first
> populate a PartScanKeyInfo from the PartClauseInfo by calling
> extract_bounding_datums().  Then it calls get_partitions_for_keys()
> which generates the set of unpruned partition indexes from the
> PartitionPruneContext and PartScanKeyInfo.

Hi Robert,

I feel I should step in here and answer this part as it was me who
first came up with the idea of the context struct. I've typed up
something below which is my first cut at what I'd have imagined the
header comment of partprune.c should look like. Some parts are only
revant after run-time pruning is also using this stuff. I've tried to
highlight those areas, I'm not sure how much or if there should be any
mention of that at all as part of this patch.

Here goes:

partprune.c

Allows efficient identification of the minimal set of partitions which match a
given set of clauses.  Thus allowing useful things such as ignoring unneeded
partitions which cannot possibly contain tuples matching the given set of
clauses.

This module breaks the process of determining the matching partitions into
two distinct steps, each of which has its own function which is externally
visible outside of this module.  The reason for not performing everything
in one step as down to the fact that there are times where we may wish to
perform the 2nd step multiple times over.  The steps could be thought of as a
compilation step followed by an execution step.

Step 1 (compilation):

Pre-process the given list of clauses and attempt to match individual clauses
up to a partition key.

The end result of this process is a PartitionClauseInfo containing details of
each clause found to match the partition key.  This output is required as
input for the 2nd step.

Step 2 (execution):

Step 2 outputs the minimal set of matching partitions based on the input from
step 1.

Internally, this step is broken down into smaller sub-steps, each of which
is explained in detail in the comments in the corresponding function.

Step 2 can be executed multiple times for its input values.  The inputs to this
step are not modified by the processing done within.  It is expected that this
step is executed multiple times in cases where the matching partitions must be
determined during query execution.  A subsequent evaluation of this step will
be required whenever a parameter which was found in a clause matching the
partition key changes its value.

PartitionPruneContext:

Each of the steps described above also requires an input of a
PartitionPruneContext.  This stores all of the remaining required inputs to
each step.  The context will vary slightly depending on the context in which
the step is being called from; i.e the planner or executor. For example,
during query planning, we're unable to determine the value of a Param found
matching the partition key.  When this step is called from the executor the
PlanState can be set in the context which allows evaluation of these Params
into Datum values. *** Only after run-time pruning ***

The PartitionPruneContext is also required since many of the query planner
node types are unavailable to the executor, which means that the source
information used to populate the context will vary depending on if it's being
called from the query planner or executor.

*** Only after run-time pruning ***

The context is also modified during step 1 to record all of the Param IDs
which were found to match the partition key.

-

Hopefully that also helps explain the intensions with the current code strucure.

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

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

[Andres Freund]

Hi,

On 2018-01-26 23:30:08 +0100, Daniel Gustafsson wrote:
> And another rebase and update after the refactoring in c1869542b3a4da4b12ca.
> Also fixed some typos in comments.  The other patches originally posted in 
> this
> patchset are either committed or made redundant.

Could you provide a quick summary about where you think this patch
stands currently? The cover letter you had for this thread was great...

- Andres

Re: PATCH: Configurable file mode mask

[Andres Freund]

Hi,

On 2018-02-27 15:52:32 -0500, David Steele wrote:
> Thanks for having a look at the patches.

I'd personally appreciate if you'd add commit messages to the individual
patches in a series. A brief explanation why something is done is good
enough.  E.g. here it's far from obvious why something is done with
pg_resetwal.

I'd suggest just using git format-patch -v to format series.


Independent of that, I'm not entirely clear on what the status of this
patch is, without rereading the thread. Could you summarize?

- Andres

Re: 2018-03 Commitfest Summary (Andres #2)

[Pavel Stehule]

>
>
> - new plpgsql extra_checks
>
>   WOA, but recently set to that status. Patch essentially from
>   2017-01-11.
>
>   I'm not really sure there's agreement we want this.
>
>
This patch is simple and has benefit for users with basic plpgsql skills,
and some for all.

In more complex cases, probably plpgsql_check is used everywhere today,
what is better but significantly complex solution. But not all cases can be
solved by plpgsql_check, because it does only static analyze. This patch
does some runtime warnings and checks.

Regards

Pavel



>
> - "Possibility to controll plpgsql plan cache behave"
>
>   NR, current incarnation is from late last year.  Note that the patch
>   doesn't at all do anymore what the subject says. It's GUCs that can
>   force custom / generic plans.
>
>   Seems simple, if we want it.
>
>
>

Re: 2018-03 CFM

[David Steele]

Hi Alexander,

On 2/27/18 5:05 AM, Aleksander Alekseev wrote:

Hello David,


Just a few days left until the last Commitfest for the PG11 release begins!

I'm planning to fill the CFM role, unless there are objections.


Thank you for volunteering!

I would like to be CFM assistant, if you need one :)


Right now Andres and I are doing triage on the patches to determine what 
is likely to be committed in this CF.


I would be very interested in your input.  I generally start with 
patches that have had no email for the longest time and work my way 
down.  You can sort on the "Latest Mail" column in the CF app to help.


I'll be working on this again in the morning, but I'm guessing you are 
6-7 hours earlier than me so if you have time in the morning have a 
look.  You can post your results to the list or send them to me.


However, at 09:00 ET I will be working again and we might overlap if you 
go into that time.


Thanks!
--
-David
da...@pgmasters.net

Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Andres Freund]

On 2018-01-17 09:03:51 -0500, Peter Eisentraut wrote:
> Graham, will you be able to respond to my questions or provide an
> updated patch within the next week or so?

Given that nothing has happend since, I've marked this as returned with
feedback.

Greetings,

Andres Freund

Re: [HACKERS] Runtime Partition Pruning

[David Rowley]

On 2 March 2018 at 07:17, Jesper Pedersen  wrote:
> A small typo in 0001:
>
> + * leftmost_ons_pos[x] gives the bit number (0-7) of the leftmost one bit
> in a
>
> ..."_one_"...

Oops. I'll fix that.

> 0004 fails "make check-world" due to
>
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 670; 1259 49954 TABLE
> boolp_f jpedersen
> pg_restore: [archiver (db)] could not execute query: ERROR:  syntax error at
> or near "false"
> LINE 24: ..." ATTACH PARTITION "public"."boolp_f" FOR VALUES IN (false);

The tests seem to have stumbled on a pg_dump bug which causes it to
produce syntax that's not valid (currently)

I should be able to stop my patch failing the test by dropping that
table, which I should have been doing anyway.

> Do you require https://commitfest.postgresql.org/17/1410/ as well ?

I'll look at that thread and see if there's any pg_dump being broken discussion.

> I'll look more at 0002-0005 over the coming days.

Thanks for the review and in advance for the future review.

I'll delay releasing a new patch as there's some discussion over on
the faster partition pruning thread which affects this too [1]

[1] 
https://www.postgresql.org/message-id/CA+Tgmoa4D1c4roj7L8cx8gkkeBWAZD=mtcxkxtwbnslrhd3...@mail.gmail.com

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

Re: [Patch] Checksums for SLRU files

[Andres Freund]

On 2018-02-02 11:37:34 +1300, Thomas Munro wrote:
> > 3. pg_upgrade isn't considered.  This patch should provide upgrading SLRUs
> > to adopt changed useful size of page.  That seems to be hardest patch of
> > this patch to be written.
> 
> +1
> 
> I think we'd want pg_upgrade tests showing an example of each SLRU
> growing past one segment, and then being upgraded, and then being
> accessed in various different pages and segment files, so that we can
> see that we're able to shift the data to the right place successfully.
> For example I think I'd want to see that a single aborted transaction
> surrounded by many committed transactions shows up in the right place
> after an upgrade.

This patch is in the 2018-03 CF, but I don't see any progress since the
last comments. As it has been Waiting on author since the last CF, I
think we should mark this as returned with feedback.

Greetings,

Andres Freund

Re: [HACKERS] logical decoding of two-phase transactions

[Craig Ringer]

On 2 March 2018 at 08:53, Andres Freund  wrote:

> Hi,
>
> On 2018-02-28 21:12:42 +0530, Nikhil Sontakke wrote:
> > Attached are 5 patches split up from the original patch that I had
> > submitted earlier.
>
> In the future you should number them. Right now they appear to be out of
> order in your email.  I suggest using git format-patch, that does all
> the necessary work for you.
>
> Yep, specially git format-patch with a -v argument, so the whole patchset
is visibly versioned and sorts in the correct order.



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

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Andres Freund]

On 2018-02-01 23:51:55 +0100, Tomas Vondra wrote:
> On 02/01/2018 03:51 PM, Peter Eisentraut wrote:
> > To close out this commit fest, I'm setting both of these patches as
> > returned with feedback, as there are apparently significant issues to be
> > addressed.  Feel free to move them to the next commit fest when you
> > think they are ready to be continued.
> > 
> 
> Will do. Thanks for the feedback.

Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
but I don't see a newer version posted?

Greetings,

Andres Freund

Re: Optimize Arm64 crc32c implementation in Postgresql

[Michael Paquier]

On Thu, Mar 01, 2018 at 01:36:23PM -0800, Andres Freund wrote:
> On 2018-01-10 15:09:16 +0900, Michael Paquier wrote:
>> There are not enough patches for ARM.
> 
> Nah, not needing arch specific patches is good ;)

You know already that I am always impressed by your skills in
normalizing things where needed.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] logical decoding of two-phase transactions

[Andres Freund]

Hi,

On 2018-02-28 21:12:42 +0530, Nikhil Sontakke wrote:
> Attached are 5 patches split up from the original patch that I had
> submitted earlier.

In the future you should number them. Right now they appear to be out of
order in your email.  I suggest using git format-patch, that does all
the necessary work for you.

Greetings,

Andres Freund

Re: Documenting PROVE_TESTS in section of TAP tests

[Michael Paquier]

On Thu, Mar 01, 2018 at 01:52:09AM -0800, Andres Freund wrote:
> Pushed, after replacing "a subset" with "the specified subset".

Thanks, Andres.
--
Michael


signature.asc
Description: PGP signature

Re: Parallel Aggregates for string_agg and array_agg

[Andres Freund]

On 2018-03-02 13:48:00 +1300, David Rowley wrote:
> On 2 March 2018 at 10:26, Andres Freund  wrote:
> > FWIW, I've heard numerous people yearn for json*agg
> 
> I guess it'll need to be PG12 for now. I'd imagine string_agg and
> array_agg are more important ones to tick off for now, but I bet many
> people would disagree with that.

Oh, this was definitely not meant as an ask for something done together
with this submission.

Thanks for the quick update.

Greetings,

Andres Freund

Re: Parallel Aggregates for string_agg and array_agg

[David Rowley]

On 2 March 2018 at 10:26, Andres Freund  wrote:
> On 2017-12-18 03:30:55 +1300, David Rowley wrote:
>> Just a handful of aggregates now don't support partial aggregation;
>>
>> postgres=# select aggfnoid from pg_aggregate where aggcombinefn=0 and
>> aggkind='n';
>>  aggfnoid
>> --
>>  xmlagg
>>  json_agg
>>  json_object_agg
>>  jsonb_agg
>>  jsonb_object_agg
>> (5 rows)
>
> FWIW, I've heard numerous people yearn for json*agg

I guess it'll need to be PG12 for now. I'd imagine string_agg and
array_agg are more important ones to tick off for now, but I bet many
people would disagree with that.

>
>> I'm going to add this to PG11's final commitfest rather than the
>> January 'fest as it seems more like a final commitfest type of patch.
>
> Not sure I understand?

hmm, yeah. That was more of a consideration that I should be working
hard on + reviewing more complex and invasive patches before the final
'fest. I saw this more as something that would gain more interest once
patches such as partition-wise aggs gets in.

>> + /* XXX is there anywhere to cache this to save calling each 
>> time? */
>> + getTypeBinaryOutputInfo(state->element_type, &typsend, 
>> &typisvarlena);
>
> Yes, you should be able to store this in fcinfo->flinfo->fn_extra. Or do
> you see a problem doing so?

No, just didn't think of it.

>> + /* XXX? Surely this cannot be the way to do this? */
>> + StringInfoData tmp;
>> + tmp.cursor = 0;
>> + tmp.maxlen = tmp.len = pq_getmsgint(&buf, 4);
>> + tmp.data = (char *) pq_getmsgbytes(&buf, tmp.len);
>> +
>> + result->dvalues[i] = 
>> OidReceiveFunctionCall(typreceive, &tmp,
>> +
>>  typioparam, -1);
>
> Hm, doesn't look too bad to me? What's your concern here?

I was just surprised at having to fake up a StringInfoData.

> This isn't a real review, I was basically just doing a quick
> scroll-through.

Understood.

I've attached a delta patch which can be applied on top of the v2
patch which removes that XXX comment above and also implements the
fn_extra caching.

Thanks for looking!

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


combinefn_for_string_and_array_aggs_v3_delta.patch
Description: Binary data

Re: [HACKERS] make async slave to wait for lsn to be replayed

[Andres Freund]

On 2018-02-02 19:41:37 +, Simon Riggs wrote:
> On 2 February 2018 at 18:46, Robert Haas  wrote:
> > On Fri, Feb 2, 2018 at 3:46 AM, Simon Riggs  wrote:
> >> In PG11, I propose the following command, sticking mostly to Ants'
> >> syntax, and allowing to wait for multiple events before it returns. It
> >> doesn't hold snapshot and will not get cancelled by Hot Standby.
> >>
> >> WAIT FOR event [, event ...] options
> >>
> >> event is
> >> LSN value
> >> TIMESTAMP value
> >>
> >> options
> >> TIMEOUT delay
> >> UNTIL TIMESTAMP timestamp
> >> (we have both, so people don't need to do math, they can use whichever
> >> they have)
> >
> > WAIT FOR TIMEOUT sounds a lot like SELECT pg_sleep_for(), and WAIT
> > UNTIL TIMESTAMP sounds a lot like SELECT pg_sleep_until().
> 
> Yes, it sounds very similar. It's the behavior that differs; I read
> and agreed with yours and Thomas' earlier comments on that point.
> 
> As pointed out upthread, the key difference is whether it gets
> cancelled on Hot Standby and whether you can call it in a non-READ
> COMMITTED transaction.

Given that nobody has updated the patch or even discussed doing so, I
assume this would CF issue should now appropriately be classified as
returned with feedback?

Greetings,

Andres Freund

Re: [HACKERS] Fix warnings and typo in dshash

[Andres Freund]

On 2018-02-12 09:23:43 +1300, Thomas Munro wrote:
> On Mon, Sep 4, 2017 at 2:18 PM, Amit Kapila  wrote:
> > On Sun, Sep 3, 2017 at 2:56 PM, Thomas Munro
> >  wrote:
> >> I think it should be (size_t) 1, not UINT64CONST(1).  See attached.
> >
> > Okay, that makes sense.  Do you think we should also change type
> > casting in BUCKETS_PER_PARTITION so that we are consistent?
> 
> +1
> 
> Here's a patch to fix that.  I was reminded to come back and tidy this
> up when I spotted a silly mistake in a nearby comment, also fixed in
> this patch.

Pushed.


Thanks,

Andres

Re: Removing shm_mq.c's volatile qualifiers

[Andres Freund]

Hi,

On 2018-02-12 12:52:32 +1300, Thomas Munro wrote:
> From e584628bb846be11a137b5216e955284dfd646a5 Mon Sep 17 00:00:00 2001
> From: Thomas Munro 
> Date: Thu, 8 Feb 2018 18:50:32 +1300
> Subject: [PATCH] Remove volatile qualifiers from shm_mq.c.
> 
> Since commit 0709b7ee, spinlock primitives include a compiler barrier so
> it is no longer necessary to access either spinlocks or the memory they
> protect through pointer-to-volatile.  Like earlier commits e93b6298, d53e3d5f,
> 430008b5, 8f6bb851, df4077cd.

Pushed.


Thanks,

Andres

Re: Rewrite of pg_dump TAP tests

[Andres Freund]

Hi,

On 2018-02-26 13:15:04 -0500, Stephen Frost wrote:
> Attached is a patch (which applies cleaning against a2a2205, but not so
> much anymore, obviously, but I will fix after the releases) which
> greatly improves the big pg_dump TAP tests.  There's probably more which
> can be done, but I expect people will be much happier with this.  The
> cliff-notes are:

Could you update?


Do you think this should be backpatched so we can backpatch tests when
backpatching fixes?

Greetings,

Andres Freund

Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

[Andres Freund]

Hi,

On 2018-02-22 19:48:46 +0300, Nikolay Shaplov wrote:
> This is part or my bigger patch 
> https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m
>  we've decided to 
> commit by smaller parts.

I've not read that thread. Is this supposed to be a first step towards
something larger?


> So for example if you set custom fillfactor value for some index, then it 
> will 
> lead to allocating 84 bytes of memory (sizeof StdRdOptions on i386) and only 
> 8 
> bytes will be actually used (varlena header and fillfactor value). 74 bytes 
> is 
> not much, but allocating them for each index for no particular reason is bad 
> idea, as I think.

I'm not sure this is a particularly strong motivator though?  Does the
patch have a goal besides saving a few bytes?


> Moreover when I wrote my big reloption refactoring patch, I came to "one 
> reloption kind - one binary representation" philosophy. It allows to write 
> code with more logic in it.

I have no idea what this means?


Are you aiming this for v11 or v12?


Greetings,

Andres Freund

Re: Fwd: [BUGS] pg_trgm word_similarity inconsistencies or bug

[David Steele]

Hi Alexander,

On 3/1/18 4:26 PM, Alexander Korotkov wrote:
On Thu, Mar 1, 2018 at 11:05 PM, David Steele > wrote:


I agree with Teodor (upthread, not quoted here) that the documentation
could use some editing.

I started to do it myself, but quickly realized I have no knowledge of
the content.  I'm afraid I would destroy the meaning while updating the
grammar.

That's to problem.  If you're willing to help you can edit the documentation
and let me review that it's correct.  Also feel free to ask any 
questions and

more explanation from me.  Ultimately, we need to have a documentation
that any average user can understand, not to mention you :)


OK, I'm the CFM so I have my plate full for the next few days but if 
nobody picks this up then I will give it a go.



Anyone understand the subject matter well enough to review the
documentation?

I expect it would be hard to find anybody matching this criteria.


You are probably right, but it never hurts to try.

--
-David
da...@pgmasters.net

Re: Hash Joins vs. Bloom Filters / take 2

[David Steele]

On 3/1/18 6:52 PM, Tomas Vondra wrote:

On 03/02/2018 12:31 AM, Andres Freund wrote:



On March 1, 2018 3:22:44 PM PST, Tomas Vondra  
wrote:



On 03/01/2018 11:01 PM, Andres Freund wrote:

Hi,

On 2018-02-20 22:23:54 +0100, Tomas Vondra wrote:

So I've decided to revive the old patch, rebase it to current

master,

and see if we can resolve the issues that killed it in 2016.


There seems to be some good discussion in the thread. But the patch
arrived just before the last commitfest and certainly isn't a trivial
cleanup patch. Therefore I think it should be moved to the next CF?



It isn't a massive invasive patch either, though, so I object to moving
it to 2018-09 right away.


Why do we have rules around not submitting large stuff to the last
cf, if we just not follow through? We're neck deep in patches that
are older. And you've already gotten a fair bit of feedback..



It was not my intention to break (or even bend) the CF rules, of course.
I haven't considered the patch to be "large stuff", while you do. I see
Peter Geoghegan agrees with your conclusion on another thread, so go
ahead and move it to 2018-09.


After reviewing the thread I also agree that this should be pushed to 
2018-09, so I have done so.


I'm very excited by this patch, though.  In general I agree with Peter 
that a higher rate of false positives is acceptable to save memory.  I 
also don't see any reason why this can't be tuned with a parameter. 
Start with a conservative default and allow the user to adjust as desired.


--
-David
da...@pgmasters.net

Re: Better Upgrades

[Bruce Momjian]

On Tue, Feb  6, 2018 at 01:51:09PM +0100, Daniel Gustafsson wrote:
> > On 06 Feb 2018, at 01:09, David Fetter  wrote:
> 
> > - pg_upgrade is very much a blocker for on-disk format changes.
> 
> I wouldn’t call it a blocker, but pg_upgrade across an on-disk format change
> would be a very different experience from what we have today since it would
> need to read and rewrite data rather than hardlink/copy.  Definitely not a
> trivial change though, that I completely agree with.

Uh, not necessarily.  To allow for on-disk format changes, pg_upgrade
_could_ rewrite the data files as it copies them (not link), or we could
modify the backend to be able to read the old format.  We have already
done that for some changes to data and index types.

If we did add more backward-read capability to the backend, I think we
would need to add some housekeeping so we could know when the
read-old-format code could be removed.

I have to admit that the number of hurdles needed to use logical
replication to reduce downtime made me feel like it is cleaner to just
take the multi-minute downtime using link mode and have it be reliable. 
Even if you can get logical replication to work, the drain/switch over
time could be significant.

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

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

Re: [HACKERS] Bug in to_timestamp().

[Dmitry Dolgov]

> On 18 February 2018 at 18:49, Dmitry Dolgov <9erthali...@gmail.com> wrote:
> > On 17 February 2018 at 10:02, Arthur Zakirov 
wrote:
> > > On Wed, Feb 14, 2018 at 05:23:53PM +0100, Dmitry Dolgov wrote:
> > >
> > > SELECT to_timestamp('2000 + JUN', ' /') FROM dual
> > > ORA-01830: date format picture ends before converting entire input
string
> > >
> > > SELECT to_timestamp('2000 + JUN', ' ') FROM dual
> > > ORA-01830: date format picture ends before converting entire input
string
> > >
> > > It's sort of corner case, but anyway maybe you would be interested to
handle
> > > it.
> >
> > I think it could be fixed by another patch. But I'm not sure that it
> > will be accepted as well as this patch :).
>
> Why do you think there should be another patch for it?

Just to make myself clear. I think it's fair enough to mark this patch as
ready
for committer - the implementation is neat and sufficient as for me, and it
provides a visible improvement for user experience. The question above is
probably the only thing that prevents me from proposing this.

Re: Hash Joins vs. Bloom Filters / take 2

[Tomas Vondra]

On 03/02/2018 12:31 AM, Andres Freund wrote:
> 
> 
> On March 1, 2018 3:22:44 PM PST, Tomas Vondra  
> wrote:
>>
>>
>> On 03/01/2018 11:01 PM, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2018-02-20 22:23:54 +0100, Tomas Vondra wrote:
 So I've decided to revive the old patch, rebase it to current
>> master,
 and see if we can resolve the issues that killed it in 2016.
>>>
>>> There seems to be some good discussion in the thread. But the patch
>>> arrived just before the last commitfest and certainly isn't a trivial
>>> cleanup patch. Therefore I think it should be moved to the next CF?
>>>
>>
>> It isn't a massive invasive patch either, though, so I object to moving
>> it to 2018-09 right away.
> 
> Why do we have rules around not submitting large stuff to the last 
> cf, if we just not follow through? We're neck deep in patches that
> are older. And you've already gotten a fair bit of feedback..
> 

It was not my intention to break (or even bend) the CF rules, of course.
I haven't considered the patch to be "large stuff", while you do. I see
Peter Geoghegan agrees with your conclusion on another thread, so go
ahead and move it to 2018-09.

regards

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

Re: [patch] BUG #15005: ANALYZE can make pg_class.reltuples inaccurate.

[Tom Lane]

Alexander Kuzmenkov  writes:
> On 01.03.2018 18:09, Tom Lane wrote:
>> Ideally, at least, the estimate would remain on-target.

> The test shows that under this particular scenario the estimated number 
> of tuples grows after each ANALYZE. I tried to explain how this happens 
> in the attached pdf.

I looked at this and don't think it really answers the question.  What
happens is that, precisely because we only slowly adapt our estimate of
density towards the new measurement, we will have an overestimate of
density if the true density is decreasing (even if the new measurement is
spot-on), and that corresponds exactly to an overestimate of reltuples.
No surprise there.  The question is why it fails to converge to reality
over time.

I think part of David's point is that because we only allow ANALYZE to
scan a limited number of pages even in a very large table, that creates
an artificial limit on the slew rate of the density estimate; perhaps
what's happening in his tables is that the true density is dropping
faster than that limit allows us to adapt.  Still, if there's that
much going on in his tables, you'd think VACUUM would be touching
enough of the table that it would keep the estimate pretty sane.
So I don't think we yet have a convincing explanation of why the
estimates drift worse over time.

Anyway, I find myself semi-persuaded by his argument that we are
already assuming that ANALYZE has taken a random sample of the table,
so why should we not believe its estimate of density too?  Aside from
simplicity, that would have the advantage of providing a way out of the
situation when the existing reltuples estimate has gotten drastically off.

The sticking point in my mind right now is, if we do that, what to do with
VACUUM's estimates.  If you believe the argument in the PDF that we'll
necessarily overshoot reltuples in the face of declining true density,
then it seems like that argument applies to VACUUM as well.  However,
VACUUM has the issue that we should *not* believe that it looked at a
random sample of pages.  Maybe the fact that it looks exactly at the
changed pages causes it to see something less than the overall density,
cancelling out the problem, but that seems kinda optimistic.

Anyway, as I mentioned in the 2011 thread, the existing computation is
isomorphic to the rule "use the old density estimate for the pages we did
not look at, and the new density estimate --- ie, exactly scanned_tuples
--- for the pages we did look at".  That still has a lot of intuitive
appeal, especially for VACUUM where there's reason to believe those page
populations aren't alike.  We could recast the code to look like it's
doing that rather than doing a moving-average, although the outcome
should be the same up to roundoff error.

regards, tom lane

Re: Hash Joins vs. Bloom Filters / take 2

[Andres Freund]

On March 1, 2018 3:22:44 PM PST, Tomas Vondra  
wrote:
>
>
>On 03/01/2018 11:01 PM, Andres Freund wrote:
>> Hi,
>> 
>> On 2018-02-20 22:23:54 +0100, Tomas Vondra wrote:
>>> So I've decided to revive the old patch, rebase it to current
>master,
>>> and see if we can resolve the issues that killed it in 2016.
>> 
>> There seems to be some good discussion in the thread. But the patch
>> arrived just before the last commitfest and certainly isn't a trivial
>> cleanup patch. Therefore I think it should be moved to the next CF?
>> 
>
>It isn't a massive invasive patch either, though, so I object to moving
>it to 2018-09 right away.

Why do we have rules around not submitting large stuff to the last cf, if we 
just not follow through? We're neck deep in patches that are older. And you've 
already gotten a fair bit of feedback..


Andres

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Hash Joins vs. Bloom Filters / take 2

[Tomas Vondra]

On 03/01/2018 11:01 PM, Andres Freund wrote:
> Hi,
> 
> On 2018-02-20 22:23:54 +0100, Tomas Vondra wrote:
>> So I've decided to revive the old patch, rebase it to current master,
>> and see if we can resolve the issues that killed it in 2016.
> 
> There seems to be some good discussion in the thread. But the patch
> arrived just before the last commitfest and certainly isn't a trivial
> cleanup patch. Therefore I think it should be moved to the next CF?
> 

It isn't a massive invasive patch either, though, so I object to moving
it to 2018-09 right away.

regards

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

Re: Faster inserts with mostly-monotonically increasing values

[Claudio Freire]

On Sun, Dec 31, 2017 at 8:06 AM, Peter Geoghegan  wrote:
> I also have my
> doubts about unique index enforcement remaining correct with the patch
> when there are many physical duplicates, to the extent that more than
> a single leaf page is needed for a single value.

given...

+if (P_ISLEAF(lpageop) && P_RIGHTMOST(lpageop) &&
+!P_INCOMPLETE_SPLIT(lpageop) &&
+!P_IGNORE(lpageop) &&
+(PageGetFreeSpace(page) > itemsz) &&
+_bt_compare(rel, natts, itup_scankey, page,
PageGetMaxOffsetNumber(page)) > 0)

The key there is strictly greater than the rightmost key. As such, it
must precede the first page with an equal key, and since it's the
rightmost page... there's no such key. But if there was, the check for
uniqueness only needs the insert point to point to the first such
page, and this guarantees it.

You could allow for some slack, by comparing against the leftmost key
instead. You just need to know whether the key fits in the page. Then,
the insert can proceed as usual, doing the binsearch to find the
proper insertion point, etc.

That would allow this case to be applied not only to perfectly
monotonic sequences, but for nearly monotonic ones as well (think
timestamps).

Re: 2018-03 Commitfest Summary (Andres #2)

[Peter Geoghegan]

()
On Thu, Mar 1, 2018 at 2:45 PM, Andres Freund  wrote:
> - "failed to find parent tuple for heap-only tuple" error as an 
> ERRCODE_DATA_CORRUPTION ereport()
>
>   NR. Should probably just get applied. Can't quite make myself care
>   enough to interrupt right now.

Tom just committed this.

> - Convert join OR clauses into UNION queries
>
>   NR. ~1 year old.
>
>   Feature has been requested several times independently since. Patch
>   has been rebased, but otherwise not meaningfully changed.
>
>   I think some committers need to bite the bullet and review this one.

I think that this is really important, too. Greg Stark expressed
interest recently, in a thread where we asked about this topic without
actually being aware of the existence of the patch. Perhaps there is
some chance he'll help with it.

> - Incremental sort
>
>   NR. This is a *very* old "patch" (obviously has evolved).
>
>   Looks to be in a reasonable shape. This needs at the very least a few
>   planner & executor skilled committer cycles to do a review so it can
>   progress.

This one has been bounced way too many times already. I really hope it
doesn't fall through the cracks for v11.

> - Faster inserts with mostly-monotonically increasing values
>
>   NR, patch from Dec, CF entry created Feb.
>
>   Nice performance improvement. Hasn't gotten that much review, and it
>   appears Peter Geoghegan has some correctness concerns.  Not sure.

I think that the idea is basically sound, but I would like to see a
justification for not caching page LSN instead of testing if the leaf
page is still rightmost and so on. If the LSN changed, that means that
the cached block number is stale. That seems simpler, and more or less
matches what we already do within _bt_killitems().

> - hash joins with bloom filters
>
>   NR, patch from 2018-02-20.
>
>   This is a major new patch, arriving just before the last CF. I think
>   this should be moved to the next fest.

I also think it should be bumped. I like this patch, but it's not
gonna happen for 11.

-- 
Peter Geoghegan

Re: Sample values for pg_stat_statements

[Vik Fearing]

On 03/01/2018 07:26 PM, Andres Freund wrote:
> Hm. Isn't this going to blow up the size of the file in cases with a
> number of parameters quite considerably, a file limit notwithstanding?
> Wonder if the size limit wouldn't have to be across all params.

It is across all params (per queryid).
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Re: row filtering for logical replication

[David Steele]

On 3/1/18 6:00 PM, Euler Taveira wrote:

2018-03-01 18:27 GMT-03:00 Andres Freund :

FWIW, I don't think it'd be fair or prudent. There's definitely some
issues (see e.g. Craig's reply), and I don't see why this patch'd
deserve an exemption from the "nontrivial patches shouldn't be submitted
to the last CF" policy?


I forgot to mention but this feature is for v12. I know the rules and
that is why I didn't add it to the in progress CF.


That was the right thing to do, thank you!

--
-David
da...@pgmasters.net

Re: row filtering for logical replication

[Euler Taveira]

2018-02-28 21:54 GMT-03:00 Craig Ringer :
> Good idea. I haven't read this yet, but one thing to make sure you've
> handled is limiting the clause to referencing only the current tuple and the
> catalogs. user-catalog tables are OK, too, anything that is
> RelationIsAccessibleInLogicalDecoding().
>
> This means only immutable functions may be invoked, since a stable or
> volatile function might attempt to access a table. And views must be
> prohibited or recursively checked. (We have tree walkers that would help
> with this).
>
> It might be worth looking at the current logic for CHECK expressions, since
> the requirements are similar. In my opinion you could safely not bother with
> allowing access to user catalog tables in the filter expressions and limit
> them strictly to immutable functions and the tuple its self.
>
IIRC implementation is similar to RLS expressions. I'll check all of
these rules.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Andres Freund]

On 2018-03-02 01:56:00 +0300, Alexander Korotkov wrote:
> On Fri, Mar 2, 2018 at 1:51 AM, Andres Freund  wrote:
> 
> > On 2018-03-02 01:48:03 +0300, Alexander Korotkov wrote:
> > > Also, the last commitfest is already too late for such big changes.
> > > So, I'm marking this RWF.
> >
> > Agreed.  Perhaps extract the 64bit GUC patch and track that separately?
> > Seems like something we should just do...
> >
> 
> Sounds reasonable.  But I didn't notice if there are other users for 64bit
> GUCs besides 64bit xids?

I think there were a couple past occasions where we could've used that,
don't quite recall the details. We're at least not that far away from
the point where various size limits are actually limited by int32
range. And timeouts of ~25 days are long but not entirely unreasonable.

Greetings,

Andres Freund

Re: row filtering for logical replication

[Euler Taveira]

2018-03-01 18:25 GMT-03:00 Erik Rijkers :
> Attached is 'logrep_rowfilter.sh', a demonstration of above-described bug.
>
Thanks for testing. I will figure out what is happening. There are
some leaks around. I'll post another version when I fix some of those
bugs.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: row filtering for logical replication

[Euler Taveira]

2018-03-01 18:27 GMT-03:00 Andres Freund :
> FWIW, I don't think it'd be fair or prudent. There's definitely some
> issues (see e.g. Craig's reply), and I don't see why this patch'd
> deserve an exemption from the "nontrivial patches shouldn't be submitted
> to the last CF" policy?
>
I forgot to mention but this feature is for v12. I know the rules and
that is why I didn't add it to the in progress CF.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Alexander Korotkov]

On Fri, Mar 2, 2018 at 1:51 AM, Andres Freund  wrote:

> On 2018-03-02 01:48:03 +0300, Alexander Korotkov wrote:
> > Also, the last commitfest is already too late for such big changes.
> > So, I'm marking this RWF.
>
> Agreed.  Perhaps extract the 64bit GUC patch and track that separately?
> Seems like something we should just do...
>

Sounds reasonable.  But I didn't notice if there are other users for 64bit
GUCs
besides 64bit xids?

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

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Andres Freund]

On 2018-03-02 01:48:03 +0300, Alexander Korotkov wrote:
> Also, the last commitfest is already too late for such big changes.
> So, I'm marking this RWF.

Agreed.  Perhaps extract the 64bit GUC patch and track that separately?
Seems like something we should just do...

Greetings,

Andres Freund