Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Amit Kapila]

On Mon, Nov 26, 2018 at 5:40 PM Vik Fearing  wrote:
>
> On 23/11/2018 00:09, Haribabu Kommi wrote:
> >
> > On Fri, Nov 23, 2018 at 1:54 AM Peter Eisentraut
> >  > > wrote:
> >
> > On 19/11/2018 06:18, Haribabu Kommi wrote:
> > > Amit suggested another option in another mail, so total viable
> > > solutions that are discussed as of now are,
> > >
> > > 1. Single API with NULL input treat as invalid value
> > > 2. Multiple API to deal with NULL input of other values
> > > 3. Single API with NULL value to treat them as current user, current
> > > database
> > >  and NULL queryid.
> > > 4. Single API with -1 as invalid value, treat NULL as no matching.
> > (Only
> > > problem
> > >  with this approach is till now -1 is also a valid queryid, but
> > setting
> > > -1 as queryid
> > > needs to be avoided.
> >
> > Can you show examples of what these would look like?
> >
> >
> > Following are some of the examples how the various options
> > work.
> >
> > Option -1:
> >
> > select pg_stat_statements_reset(NULL,NULL,NULL);
> > -- No change, just return. Because the function is strict.
> >
> > select pg_stat_statements_reset(10,10,NULL);
> > -- Resets all the statements that are executed by userid 10 on database
> > id 10.
> >
> > select pg_stat_statements_reset(NULL,10,NULL);
> > -- Resets all the statements executed on database id 10
>
> If the function is strict, none of these will do anything.
>
> My preference is for NULL to mean *all* so this is my favorite option,
> except that the first query should reset everything.
>
> +1 with that change.
>

Thanks, Vik for sharing feedback.  So, here is a summarization of what
different people think:

Option-1: Vik
Option-2: Alvaro, Magnus
Option-3: Michael, Hari
Option-4: Amit, Hari, Magnus


We still can't conclude clearly, but I think Option-2 and Option-4 are
the ones which appear to be more popular.  Does anyone else want to
weigh in here?  If no more people share an opinion, I will go with one
among Option-2 or Option-4.

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

RE: idle-in-transaction timeout error does not give a hint

[Ideriha, Takeshi]

>From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
>Sent: Wednesday, November 28, 2018 12:18 PM
>To: pgsql-hackers@lists.postgresql.org
>Subject: idle-in-transaction timeout error does not give a hint
>
>idle-in-transaction timeout error closed the session. I think in this case the 
>error
>message should give a hint something like other errors (for example
>ERRCODE_CRASH_SHUTDOWN or
>ERRCODE_T_R_SERIALIZATION_FAILURE) to ask users to reconnect.
>Attached patch does that.

Hi, it makes sense to me. One can submit transaction again same as other cases
you mentioned. 

I didn't attach the patch but according to my simple experiment
in psql the output would become the following:

FATAL:  terminating connection due to idle-in-transaction timeout
HINT: In a moment you should be able to reconnect to the
  database and repeat your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

Regards,
Takeshi Ideriha

psql --csv and other parameters

[Erik Rijkers]

I don't know if this really is a bug but it seems wrong to me:

psql -A --csv -Xc "select * from pg_namespace order by 1"

gives a difference result than

psql --csv -A -Xc "select * from pg_namespace order by 1"


I would say both should give the same result, and
that result should be the same as from:

psql --csv -Xc "select * from pg_namespace order by 1"


Thanks,

Erik Rijkers

Unnecessary asterisk in comment in postgres_fdw.c

[Etsuro Fujita]

Here is a small patch for removing $SUBJECT.

Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index a5830bb..d22c974 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2887,7 +2887,7 @@ estimate_path_cost_size(PlannerInfo *root,
 
 			/*-
 			 * Startup cost includes:
-			 *	  1. Startup cost for underneath input * relation
+			 *	  1. Startup cost for underneath input relation
 			 *	  2. Cost of performing aggregation, per cost_agg()
 			 *	  3. Startup cost for PathTarget eval
 			 *-

Re: Minor typo

[Kyotaro HORIGUCHI]

This is a noise from a Japanese having poor English skill..

At Wed, 28 Nov 2018 10:01:36 +0900, Michael Paquier  wrote 
in <20181128010136.gu1...@paquier.xyz>
> On Tue, Nov 27, 2018 at 07:51:03PM -0500, Stephen Frost wrote:
> > That isn't at all what I got from that.
> > 
> > A rewrite of this really should avoid talking about removing the hole as
> > if it's 'compression' because, first of all, it isn't, and second, now
> > that we have *actual* compression happening, it's terribly confusing to
> > talk about removing the hole using that terminology.
> 
> You may want to be careful about other comments in xloginsert.c or such
> then.  Removing a page hole is mentioned in a couple of places as a form
> of compression if I recall correctly.

org> When wal_compression is enabled, a full page image which "hole" was
org> removed is additionally compressed using PGLZ compression algorithm.

Even though it has an obvious mistake, I could read this as
full_page_image is always compressed after removing a hole in it
if any. (I'm not sure it makes correct sense, though.) I feel
hopelessly that the sentence structure model in my mind is
different from that of natives. I need to study harder, but..

Sorry for the noise in advance.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Planning time of Generic plan for a table partitioned into a lot

[Amit Langote]

On 2018/11/28 13:46, Amit Langote wrote:
> It's cheaper than using a cached generic plan (without re-planning),
> because the latter has to pay the cost of AcquireExecutorLocks which takes
> longer as the number of partitions increases.  Perhaps something to try
> fix fixing too.  Not planning should cost less than planning! :)

Ah, I see that David has already thought about this issue.

(last paragraph of this email)
https://www.postgresql.org/message-id/CAKJS1f-ibmyn1W_UsdSmygjKOL6YgPyX0Mz54V_iD0HWWL_h%3Dg%40mail.gmail.com

Thanks,
Amit

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Tom Lane]

Thomas Munro  writes:
> On Wed, Nov 28, 2018 at 5:28 PM Tom Lane  wrote:
>> So my theory is we broke something in HEAD a couple weeks ago.  But what?

> Hmm.  Not seeing it.  I'm trying to do it again, with a make check loop.

>> The fsync changes you made are suspiciously close to this issue (ie one
>> could explain it as written data not getting out), and were committed in
>> the right time frame, but that change didn't affect writes to
>> postmaster.pid did it?

> Commit 9ccdd7f6 doesn't affect writes to anything.  It just changes
> the elevel if certain fsync calls fail (and incidentally none near
> this code, and in any case there was no failure).

Yeah ... the other weird thing about this is that the postmaster log
shows normal shutdown.

Is it possible that unlink() on APFS is not atomic?  That is, the
sequence of events is something like

pg_ctl: open("postmaster.pid")
 postmaster: unlink("postmaster.pid")
pg_ctl: reads file, gets zero bytes

But that idea is in the OS-bug class, which we agree seems unlikely.

regards, tom lane

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Tatsuro Yamada]

On 2018/11/28 13:14, Kyotaro HORIGUCHI wrote:

Hello.

At Wed, 28 Nov 2018 11:27:23 +0900, Tatsuro Yamada  
wrote in 

Hi,

On 2018/11/26 11:05, Tatsuro Yamada wrote:
I couldn't write patches details on previous email, so I write
more explanation for that on this email.


* tab_completion_alter_index_set_statistics.patch
===
There are two problems. You can use these DDL before testing.
   #create table hoge (a integer, b integer);
   #create index ind_hoge on hoge (a, (a + b), (a * b));

   1) Can't get column names

 # alter index ind_hoge alter column ... but can't complete.


Currently the only continueable rule to the rule is SET
STATISTICS so we usually expect the number of an expression
column there. Even though we actually name every expression
column in an index, users hardly see the names. The names are in
the index column number order in your example, but what if the
name of the first column were 'foo'?

=# alter index ind_hoge2 alter column
expr   expr1  foo

We could still *guess* what is expr or exrp1 but I don't think it
helps much. (Note: foo is not usable in this context as it's a
non-expression column.)


Thanks for your comment.
We can get column name by using "\d index_name" like this:

# \d ind_hoge
   Index "public.ind_hoge"
 Column |  Type   | Key? | Definition
+-+--+
 a  | integer | yes  | a
 expr   | integer | yes  | (a + b)
 expr1  | integer | yes  | (a * b)
btree, for table "public.hoge"

So, I suppose that it's easy to understand what column is an expression column.
Of course, user will get syntax error if user chose "a" column like a "foo" 
which is
non-expression column as you mentioned.
Probably, I will be able to fix the patch to get only expression columns from 
the index.
Should I do that?


Other example, if user wants to use column number, I suppose that user have to 
check a
definition of index and count the number of columns.


# create table hoge2(a integer, b integer, foo integer);
CREATE TABLE

# create index ind_hoge2 on hoge2((a+b), foo, (a*b));
CREATE INDEX
[local] postgres@postgres:9912=# \d ind_hoge2
   Index "public.ind_hoge2"
 Column |  Type   | Key? | Definition
+-+--+
 expr   | integer | yes  | (a + b)
 foo| integer | yes  | foo
 expr1  | integer | yes  | (a * b)
btree, for table "public.hoge2"

# alter index ind_hoge2 alter column 1 set statistics 1;
ALTER INDEX

# alter index ind_hoge2 alter column 2 set statistics 1;
ERROR:  cannot alter statistics on non-expression column "foo" of index 
"ind_hoge2"

# alter index ind_hoge2 alter column 3 set statistics 1;
ALTER INDEX


I prefer to use column name instead column number because
there is no column number on \d index_name and \d+ index_name.




   2) I expected column names for column numbers after "SET STATISTICS",
   but
  tab-completion gave schema names

 # alter index ind_hoge alter column expr SET STATISTICS 
 information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
 pg_toast_temp_1.  public.


This is the result of STATISTICS  completion. SET
STATISTICS always doesn't take statistics name so this is safe.


:)


Thanks,
Tatsuro Yamada
NTT Open Source Software Center

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Thomas Munro]

On Wed, Nov 28, 2018 at 5:28 PM Tom Lane  wrote:
> Thomas Munro  writes:
> > Today I saw a one-off case of $SUBJECT, on macOS.  I can't reproduce
> > it, but I noticed exactly the same thing on longfin the other day:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-25%2005%3A39%3A04
>
> I trawled the buildfarm logs and discovered a second instance of exactly
> the same thing:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-19%2018%3A37%3A00
>
> There have not been any other occurrences in the past 3 months, which is
> as far back as I went.  (lorikeet has half a dozen occurrences of "could
> not stop postmaster", which is what I was grepping for, but they all
> are associated with that machine's intermittent postmaster crashes.)
>
> So that lets out the flaky-hardware theory: that occurrence is before
> longfin's hardware transplant.
>
> Also, I don't think I believe the OS-bug idea either, given that you
> saw it on 10.14.0.  longfin's been running 10.14.something since
> 2018-09-26, and has accumulated circa 200 runs since then just on HEAD,
> never mind the back branches.  It'd be pretty unlikely to see it only
> in the past week, and only on HEAD, if it were an OS bug introduced two
> months ago.

Yeah, it'd be slightly easier to believe when High Sierra first came
out and every hfs+ volume was silently migrated to the brand new apfs.
But yeah, that idea seems like a long shot at this point.

> So my theory is we broke something in HEAD a couple weeks ago.  But what?

Hmm.  Not seeing it.  I'm trying to do it again, with a make check loop.

> The fsync changes you made are suspiciously close to this issue (ie one
> could explain it as written data not getting out), and were committed in
> the right time frame, but that change didn't affect writes to
> postmaster.pid did it?

Commit 9ccdd7f6 doesn't affect writes to anything.  It just changes
the elevel if certain fsync calls fail (and incidentally none near
this code, and in any case there was no failure).

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

RE: Copy data to DSA area

[Ideriha, Takeshi]

Hi

>From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com]
>Sent: Wednesday, November 14, 2018 9:50 AM
>To: Ideriha, Takeshi/出利葉 健 
>
>On Tue, Nov 13, 2018 at 10:59 PM Ideriha, Takeshi 
>
>wrote:
>> Can I check my understanding?
>> The situation you are talking about is the following:
>> Data structure A and B will be allocated on DSA. Data A has a pointer to B.
>> Another data X is already allocated on some area (might be on local
>> heap) and has a pointer variable X->a, which will be linked to A.
>>
>>  old_context = MemoryContextSwitchTo(dsa_memory_context);
>>  A = palloc();
>>  B = palloc();
>>  A->b = B;
>>  X->a = A;
>>  MemoryContextSwitchTo(old_context);
>>
>> If error happens in the way of this flow, palloc'd data (A & B) should
>> be freed and pointer to freed data (X->a) should be back to its original one.
>> So handling these cases introduces an "transaction" API like 
>> begin_allocate() and
>end_allocate().
>
>I wasn't thinking of resetting X->a to its original value, just freeing A and 
>B.  For a
>shared cache in this memory area, you need to make sure that you never leak
>memory; it must either be referenced by the cache book keeping data structures 
>so
>that you can find it and manually free it eventually (probably with an LRU 
>system?), or
>it must somehow be cleaned up if an error occurs before you manage to insert 
>it into
>the cache.  During construction of an object graph, before you attach it to 
>your
>manual book keeping data structures, there is a danger zone.
>
>For example, what if, while copying a query plan or a catcache entry, we 
>successfully
>allocate a new cache entry with palloc(), but then another palloc() call fails 
>because
>we run out of memory when copying the keys?  It looks like the current 
>catcache.c
>coding just doesn't
>care: memory will be leaked permanently in CacheMemoryContext.  That's OK
>because it is process-local.  Such a process is probably doomed anyway.  If 
>you exit
>and then start a new backend the problem is gone.
>Here we need something better, because once this new kind of memory fills up 
>with
>leaked objects, you have to restart the whole cluster to recover.

Thank you for the clarification. I agree that leaving memory leaking in shared 
memory 
without freeing it ends up cluster-wide problem. 

>If the solution involves significantly different control flows (like PG_TRY(),
>PG_FINALLY() manual cleanup all over the place), then lots of existing 
>palloc-based
>code will need to be reviewed and rewritten very carefully for use in this new 
>kind of
>memory context.  If it can be done automagically, then more of our existing 
>code will
>be able to participate in the construction of stuff that we want to put in 
>shared
>memory.

About resetting X->a I was thinking about how to treat dangling pointer inside 
a new memory
context machinery instead of using PG_TRY() stuff. That's because putting 
PG_TRY() all over the 
place becomes responsible for a developer trying to shared-memory-version 
MemoryContext and 
it needs a lot of efforts as you noted. But in my mind even if PG_TRY() is 
used, it only affects new
code uses shared-memory version MemoryContext and doesn't affect current code.
I may be missing something here...

>I first thought about this in the context of shared plan caches, a topic that 
>appears on
>this mailing list from time to time.  There are some other fun problems, like 
>cache
>invalidation for shared caches, space recycling (LRUs etc), and of course
>locking/concurrency
>(dsa_allocate() and dsa_free() are concurrency safe, but whatever data 
>structures
>you build with the memory of course need their own plan for dealing with 
>concurrency).
>But a strategy for clean-up on errors seems to be a major subproblem to deal 
>with
>early in the project.  I will be very happy if we can come up with something 
>:-)

Yeah, I hope we can do it somehow.


>On Wed, Nov 14, 2018 at 12:40 AM Ideriha, Takeshi 
>
>wrote:
>> From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com]
>> >postgres=# call hoge_list(3);
>> >NOTICE:  Contents of list:
>> >NOTICE:   1
>> >NOTICE:   2
>> >NOTICE:   3
>> >CALL
>> >
>> >You can call that procedure from various different backends to add
>> >and remove integers from a List.  The point is that it could just as
>> >easily be a query plan or any other palloc-based stuff in our tree, and the 
>> >pointers
>work in any backend.
>>
>> Thanks for the patch. I know it's a rough sketch but basically that's what I 
>> want to
>do.
>> I tried it and it works well.
>
>After your message I couldn't resist a small experiment.  But it seems there 
>are
>plenty more problems to be solved to make it useful...

Thank you for kind remark. 
By the way I just thought meta variable "hoge" is used only in Japan :)

Regards,
Takeshi Ideriha

Re: vacuum and autovacuum - is it good to configure the threshold at TABLE LEVEL?

[amul sul]

On Wed, Nov 28, 2018 at 9:11 AM rajan  wrote:
>
> Thanks, amul. I have already gone through this. What I would like to
> understand is the performance impact on autovacuum launcher and worker
> process when autovacuum is running from configurations done by
> *ALTER TABLE autvac_test SET (autovacuum_vacuum_scale_factor = 0,
> autovacuum_vacuum_threshold = 100);*
>  at table level.

An answer could be yes or no, something work for me that not necessarily work
for you.

The aforesaid configuration will trigger vacuum at every 150 row update/delete.
It depends on your server load, how frequent 150 row count reaches. Also,
triggering vacuum too frequently is also not that much beneficial, IMO.


Regards,
Amul

Re: Planning time of Generic plan for a table partitioned into a lot

[Amit Langote]

Hi Kato-san,

On 2018/11/27 19:05, Kato, Sho wrote:
> Of course, in case of plan_cache_mode = force_custom_plan, it is not problem 
> because unnecessary paths are pruned by speeding up planning with partitions 
> patch[1].
> 
> However, if plan_cachemode is force_generic_plan, generic plan is made at the 
> first execution of prepared statement.
> If plan_cache_mode is auto(default), generic plan is made at the sixth 
> execution.
> So, with default setting, performance get lower at the sixth execution.

Keeping aside the fact that making a generic plan gets increasing more
expensive as the number of partitions increases, I'm a bit surprised that
you get a generic plan with plan_cache_mode = auto.  Isn't a generic plan
way too expensive in this case?

When I try your example, I always get a custom plan, because its cost is
pretty low and obviously so because it will contain only 1 partition and
even adding the cost of planning doesn't make it grow beyond a generic
plan's cost which contains 8192 partitions.  The following formula is used
to calculate the planning cost:

planning time = 1000.0 * cpu_operator_cost * (nrelations + 1)

where nrelations is the number of relations in the range table.

Here's what I get with various settings of plan caching.

--- force generic plan to see its cost

set plan_cache_mode = 'force_generic_plan';
set max_parallel_workers_per_gather = 0;
explain (timing off, analyze) execute select_stmt(8192);
  QUERY PLAN

──
 Append  (cost=0.00..343572.48 rows=106496 width=4) (actual rows=0 loops=1)
   Subplans Removed: 8191
   ->  Seq Scan on t_8192  (cost=0.00..41.88 rows=13 width=4) (actual
rows=0 loops=1)
 Filter: (id = $1)
 Planning Time: 1217.543 ms
 Execution Time: 1.340 ms
(6 rows)

-- now look at the custom plan's cost

reset plan_cache_mode; -- resets to 'auto'

explain (timing off, analyze) execute select_stmt(8192);
  QUERY PLAN

──
 Append  (cost=0.00..41.94 rows=13 width=4) (actual rows=0 loops=1)
   ->  Seq Scan on t_8192  (cost=0.00..41.88 rows=13 width=4) (actual
rows=0 loops=1)
 Filter: (id = 8192)
 Planning Time: 525.501 ms
 Execution Time: 1.104 ms
(5 rows)

So, the cost of custom plan is 41.94 + 1000 * 0.0025 * 8195 = 20529.44,
which is way less than 343572 (the generic plan cost).

-- force it to generic plan again to use the cached plan (no re-planning!)

set plan_cache_mode = 'force_generic_plan';

explain (timing off, analyze) execute select_stmt(8192);
  QUERY PLAN

──
 Append  (cost=0.00..343572.48 rows=106496 width=4) (actual rows=0 loops=1)
   Subplans Removed: 8191
   ->  Seq Scan on t_8192  (cost=0.00..41.88 rows=13 width=4) (actual
rows=0 loops=1)
 Filter: (id = $1)
 Planning Time: 14.202 ms
 Execution Time: 1.841 ms
(6 rows)

You can see that the total time is the least when a cached plan is used,
which is only possible if a generic plan can be used (even if creating it
for the first time is very expensive.).  But its cost prevents it from
being automatically selected (plan_cache_mode = 'auto').  That may be one
thing we could fix in the future by considering run-time pruning in the
equation of Append costing, so that its cost is more or less the same as
the custom plan's cost.


Just as one more data point, if you apply the patch that you mentioned
[1], you can see that custom planning costs even less than that.

reset plan_cache_mode;

explain (timing off, analyze) execute select_stmt(8192);
  QUERY PLAN

───
 Append  (cost=0.00..41.94 rows=13 width=4) (actual rows=0 loops=1)
   ->  Seq Scan on t_8192  (cost=0.00..41.88 rows=13 width=4) (actual
rows=0 loops=1)
 Filter: (id = 8192)
 Planning Time: 0.438 ms
 Execution Time: 0.121 ms
(5 rows)

It's cheaper than using a cached generic plan (without re-planning),
because the latter has to pay the cost of AcquireExecutorLocks which takes
longer as the number of partitions increases.  Perhaps something to try
fix fixing too.  Not planning should cost less than planning! :)

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/9d7c5112-cb99-6a47-d3be-cf1ee6862...@lab.ntt.co.jp

Re: postgres_fdw: oddity in costing aggregate pushdown paths

[Etsuro Fujita]

(2018/11/27 21:55), Etsuro Fujita wrote:
> While working on [1], I noticed that since we don't set the selectivity
> and cost of the local_conds (i.e., fpinfo->local_conds_sel and
> fpinfo->local_conds_cost) properly in add_foreign_grouping_paths and
> foreign_grouping_ok, estimate_path_cost_size produces considerably
> underestimated results when use_remote_estimate.  I think this would
> cause problems in later planning steps.  So, why not add something like
> this to add_foreign_grouping_paths, as done in other functions such as
> postgresGetForeignJopinPaths?
> 
> --- a/contrib/postgres_fdw/postgres_fdw.c
> +++ b/contrib/postgres_fdw/postgres_fdw.c
> @@ -5496,6 +5496,19 @@ add_foreign_grouping_paths(PlannerInfo *root,
> RelOptInfo\
>   *input_rel,
>  if (!foreign_grouping_ok(root, grouped_rel, extra->havingQual))
>  return;
> 
> +   /*
> +* Compute the selectivity and cost of the local_conds, so we don't have
> +* to do it over again for each path.  The best we can do for these
> +* conditions is to estimate selectivity on the basis of local
> statistics.
> +*/
> +   fpinfo->local_conds_sel = clauselist_selectivity(root,
> +fpinfo->local_conds,
> +0,
> +JOIN_INNER,
> +NULL);
> +
> +   cost_qual_eval(&fpinfo->local_conds_cost, fpinfo->local_conds, root);
> +
>  /* Estimate the cost of push down */
>  estimate_path_cost_size(root, grouped_rel, NIL, NIL,&rows,
>  &width,&startup_cost,&total_cost);

Here is a patch for that.

BTW another thing I noticed is this comment on costing aggregate
pushdown paths using local statistics in estimate_path_cost_size:

 * Also, core does not care about costing HAVING expressions and
 * adding that to the costs.  So similarly, here too we are not
 * considering remote and local conditions for costing.

I think this was true when aggregate pushdown went in, but isn't anymore
because of commit 7b6c07547190f056b0464098bb5a2247129d7aa2.  So we
should update estimate_path_cost_size so that it accounts for the
selectivity and cost of the HAVING expressions as well?  I think this
comment needs to be updated at least, though.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3209,3214  select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
--- 3209,3216 
   Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE (("C 1" < 100)) AND ((c2 = 6))
  (6 rows)
  
+ -- Update local stats on ft2
+ ANALYZE ft2;
  -- Add into extension
  alter extension postgres_fdw add operator class my_op_class using btree;
  alter extension postgres_fdw add function my_op_cmp(a int, b int);
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 5496,5501  add_foreign_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
--- 5496,5514 
  	if (!foreign_grouping_ok(root, grouped_rel, extra->havingQual))
  		return;
  
+ 	/*
+ 	 * Compute the selectivity and cost of the local_conds, so we don't have
+ 	 * to do it over again for each path.  The best we can do for these
+ 	 * conditions is to estimate selectivity on the basis of local statistics.
+ 	 */
+ 	fpinfo->local_conds_sel = clauselist_selectivity(root,
+ 	 fpinfo->local_conds,
+ 	 0,
+ 	 JOIN_INNER,
+ 	 NULL);
+ 
+ 	cost_qual_eval(&fpinfo->local_conds_cost, fpinfo->local_conds, root);
+ 
  	/* Estimate the cost of push down */
  	estimate_path_cost_size(root, grouped_rel, NIL, NIL, &rows,
  			&width, &startup_cost, &total_cost);
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***
*** 807,812  create operator class my_op_class for type int using btree family my_op_family a
--- 807,815 
  explain (verbose, costs off)
  select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6 and c1 < 100 group by c2;
  
+ -- Update local stats on ft2
+ ANALYZE ft2;
+ 
  -- Add into extension
  alter extension postgres_fdw add operator class my_op_class using btree;
  alter extension postgres_fdw add function my_op_cmp(a int, b int);

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Tom Lane]

Thomas Munro  writes:
> Today I saw a one-off case of $SUBJECT, on macOS.  I can't reproduce
> it, but I noticed exactly the same thing on longfin the other day:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-25%2005%3A39%3A04

I trawled the buildfarm logs and discovered a second instance of exactly
the same thing:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-19%2018%3A37%3A00

There have not been any other occurrences in the past 3 months, which is
as far back as I went.  (lorikeet has half a dozen occurrences of "could
not stop postmaster", which is what I was grepping for, but they all
are associated with that machine's intermittent postmaster crashes.)

So that lets out the flaky-hardware theory: that occurrence is before
longfin's hardware transplant.

Also, I don't think I believe the OS-bug idea either, given that you
saw it on 10.14.0.  longfin's been running 10.14.something since
2018-09-26, and has accumulated circa 200 runs since then just on HEAD,
never mind the back branches.  It'd be pretty unlikely to see it only
in the past week, and only on HEAD, if it were an OS bug introduced two
months ago.

So my theory is we broke something in HEAD a couple weeks ago.  But what?

The fsync changes you made are suspiciously close to this issue (ie one
could explain it as written data not getting out), and were committed in
the right time frame, but that change didn't affect writes to
postmaster.pid did it?

regards, tom lane

Re: Planning time of Generic plan for a table partitioned into a lot

[David Rowley]

On Tue, 27 Nov 2018 at 23:05, Kato, Sho  wrote:
> I found that making a generic plan of SELECT/UPDATE/DELETE for a table 
> partitioned into thousands is slow.
> Especially, UPDATE/DELETE statement is too slow.

It's quite well known and also documented [1] that this is slow. The
manual reads:

"Currently, pruning of partitions during the planning of an UPDATE or
DELETE command is implemented using the constraint exclusion method
(however, it is controlled by the enable_partition_pruning rather than
constraint_exclusion) — see the following section for details and
caveats that apply."

and later on the same page:

"All constraints on all children of the parent table are examined
during constraint exclusion, so large numbers of children are likely
to increase query planning time considerably. So the legacy
inheritance based partitioning will work well with up to perhaps a
hundred child tables; don't try to use many thousands of children."

That documentation should be getting adjusted by [2] as that patch
aims to improve the performance of UPDATE/DELETE planning, and also
improve planning performance for when partitions are pruned. Although
I'm not sure this will do much for you SELECT case, since you're not
pruning any partitions during planning.  There's been a discussion in
[3] about improving the performance of determining the relation's
size, which is known to be quite a bottleneck when generating a plan
which includes a partitioned table with a large number of partitions.

> I'm afraid that I could not come up with a good idea, but how can I shorten 
> the creation time of a generic plan?

Since you mentioned the plan_cache_mode GUC, then I assume you're not
talking about any version of PostgreSQL that's been released, so if
you're looking for a way to make it faster in master then I'd suggest
helping with the review of [2]. If that patch does not meet your needs
then also help Thomas with [3]. If that's still not good enough then
you might need to do some research yourself. perf is your friend
there.

[1] 
https://www.postgresql.org/docs/devel/ddl-partitioning.html#DDL-PARTITIONING-DECLARATIVE
[2] https://commitfest.postgresql.org/20/1778/
[3] 
https://www.postgresql.org/message-id/flat/CAMyN-kCPin_stCMoXCVCq5J557e9-WEFPZTqdpO3j8wzoNVwNQ%40mail.gmail.com#e085c43b597b2775326afd9f3a2b6591

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

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Kyotaro HORIGUCHI]

Hello.

At Wed, 28 Nov 2018 11:27:23 +0900, Tatsuro Yamada 
 wrote in 

> Hi,
> 
> On 2018/11/26 11:05, Tatsuro Yamada wrote:
> I couldn't write patches details on previous email, so I write
> more explanation for that on this email.
> 
> 
> * tab_completion_alter_index_set_statistics.patch
> ===
> There are two problems. You can use these DDL before testing.
>   #create table hoge (a integer, b integer);
>   #create index ind_hoge on hoge (a, (a + b), (a * b));
> 
>   1) Can't get column names
> 
> # alter index ind_hoge alter column ... but can't complete.

Currently the only continueable rule to the rule is SET
STATISTICS so we usually expect the number of an expression
column there. Even though we actually name every expression
column in an index, users hardly see the names. The names are in
the index column number order in your example, but what if the
name of the first column were 'foo'?

=# alter index ind_hoge2 alter column 
expr   expr1  foo

We could still *guess* what is expr or exrp1 but I don't think it
helps much. (Note: foo is not usable in this context as it's a
non-expression column.)

>   2) I expected column names for column numbers after "SET STATISTICS",
>   but
>  tab-completion gave schema names
> 
> # alter index ind_hoge alter column expr SET STATISTICS 
> information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
> pg_toast_temp_1.  public.

This is the result of STATISTICS  completion. SET
STATISTICS always doesn't take statistics name so this is safe.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: vacuum and autovacuum - is it good to configure the threshold at TABLE LEVEL?

[rajan]

Thanks, amul. I have already gone through this. What I would like to
understand is the performance impact on autovacuum launcher and worker
process when autovacuum is running from configurations done by
*ALTER TABLE autvac_test SET (autovacuum_vacuum_scale_factor = 0,
autovacuum_vacuum_threshold = 100);*
 at table level.



-
--
Thanks,
Rajan.
--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Thomas Munro]

On Wed, Nov 28, 2018 at 4:10 PM Tom Lane  wrote:
> Thomas Munro  writes:
> > Today I saw a one-off case of $SUBJECT, on macOS.  I can't reproduce
> > it, but I noticed exactly the same thing on longfin the other day:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-25%2005%3A39%3A04
> > Anyone know what that's about?
>
> No :-(.  I was wondering about that longfin failure too.  What macOS
> version are you running?
>
> (longfin is up to date, 10.14.1.  Is it worth noting that I jacked
> up the name and drove new hardware underneath it just last week?
> Hard to see how that could be related, but ...)

I have not yet accepted the offer to update to 10.14.1, so I guess it
must be 10.14.0, though the about box just says 10.14.  According to
sw_vers, it's 10.14 build 18A391.

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

idle-in-transaction timeout error does not give a hint

[Tatsuo Ishii]

idle-in-transaction timeout error closed the session. I think in this
case the error message should give a hint something like other errors
(for example ERRCODE_CRASH_SHUTDOWN or
ERRCODE_T_R_SERIALIZATION_FAILURE) to ask users to reconnect.
Attached patch does that.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a3b9757565..c3e4380603 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3138,7 +3138,9 @@ ProcessInterrupts(void)
 		if (IdleInTransactionSessionTimeout > 0)
 			ereport(FATAL,
 	(errcode(ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT),
-	 errmsg("terminating connection due to idle-in-transaction timeout")));
+	 errmsg("terminating connection due to idle-in-transaction timeout"),
+	 errhint("In a moment you should be able to reconnect to the"
+			 " database and repeat your command.")));
 		else
 			IdleInTransactionSessionTimeoutPending = false;
 

Re: "pg_ctl: the PID file ... is empty" at end of make check

[Tom Lane]

Thomas Munro  writes:
> Today I saw a one-off case of $SUBJECT, on macOS.  I can't reproduce
> it, but I noticed exactly the same thing on longfin the other day:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-25%2005%3A39%3A04
> Anyone know what that's about?

No :-(.  I was wondering about that longfin failure too.  What macOS
version are you running?

(longfin is up to date, 10.14.1.  Is it worth noting that I jacked
up the name and drove new hardware underneath it just last week?
Hard to see how that could be related, but ...)

regards, tom lane

Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed data

[Andres Freund]

Hi,

On 2018-11-28 03:06:58 +0100, Petr Jelinek wrote:
> On 28/11/2018 02:14, Andres Freund wrote:
> > On 2018-11-28 02:04:18 +0100, Tomas Vondra wrote:
> >> Pushed and backpatched to 9.4- (same as e9edc1ba).
> > 
> > Backpatching seems on the more aggressive end of things for an
> > optimization. Could you at least announce that beforehand next time?
> > 
> 
> Well, it may be optimization, but from what I've seen the problems
> arising from this can easily prevent logical replication from working
> altogether as reorder buffer hits OOM on bigger tables. So ISTM that it
> does warrant backpatch.

I think that's a fair argument to be made. But it should be made both
before the commit and in the commit message.

Greetings,

Andres Freund

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Tatsuro Yamada]

Hi,

On 2018/11/26 11:05, Tatsuro Yamada wrote:

Hi,

Attached patches are following:

* tab_completion_alter_index_set_statistics.patch
     - Add column name completion after ALTER COLUMN
     - Avoid schema completion after SET STATISTICS

* fix_manual_of_alter_index.patch
     - ALTER INDEX ALTER COLUMN syntax is able to use not only
   column number but also column name. So, I fixed the manual.

* tab_completion_alter_table_set_statistics.patch
     - Avoid schema completion after SET STATISTICS


I couldn't write patches details on previous email, so I write
more explanation for that on this email.


* tab_completion_alter_index_set_statistics.patch
===
There are two problems. You can use these DDL before testing.
  #create table hoge (a integer, b integer);
  #create index ind_hoge on hoge (a, (a + b), (a * b));

  1) Can't get column names

# alter index ind_hoge alter column ... but can't complete.

  2) I expected column names for column numbers after "SET STATISTICS", but
 tab-completion gave schema names

# alter index ind_hoge alter column expr SET STATISTICS 
information_schema.  pg_catalog.  pg_temp_1.   pg_toast.
pg_toast_temp_1. public.

Applied the patch:

  1) We can get column names after "alter column".

# alter index ind_hoge alter column 
a  expr   expr1

  2) Fix!
# alter index ind_hoge alter column expr SET STATISTICS 
===


* fix_manual_of_alter_index_v2.patch (Attached on this email)
===
https://www.postgresql.org/docs/devel/sql-alterindex.html

The patch fixes the syntax on above manual.
  s/column_number/column_number \| column_name/
And also it adds an explanation for column_name.

Syntax of ALTER INDEX ALTER COLUMN SET STATISTICS on the manual is below:

  ALTER INDEX [ IF EXISTS ] name ALTER [ COLUMN ] column_number
  SET STATISTICS integer

But we can use not only column number but also column name like this:

  # alter index ind_hoge alter column 2 SET STATISTICS 100;
  ALTER INDEX
  # alter index ind_hoge alter column expr SET STATISTICS 100;
  ALTER INDEX
===


* tab_completion_alter_table_set_statistics.patch
===
For now, we can get schema name by tab-completion. It is not appropriate.

  # alter table hoge alter column a set statistics 
  information_schema.  pg_catalog.  pg_temp_1.   pg_toast.  
  pg_toast_temp_1. public.

Applied the patch:

  # alter table hoge alter column a set statistics 
===

Any feedback is welcome. :)

Thanks,
Tatsuro Yamada
NTT Open Source Software Center
diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml
index 6d34dbb74e..1b5b1f7ef1 100644
--- a/doc/src/sgml/ref/alter_index.sgml
+++ b/doc/src/sgml/ref/alter_index.sgml
@@ -27,7 +27,7 @@ ALTER INDEX name ATTACH PARTITION <
 ALTER INDEX name DEPENDS ON EXTENSION extension_name
 ALTER INDEX [ IF EXISTS ] name SET ( storage_parameter = value [, ... ] )
 ALTER INDEX [ IF EXISTS ] name RESET ( storage_parameter [, ... ] )
-ALTER INDEX [ IF EXISTS ] name ALTER [ COLUMN ] column_number
+ALTER INDEX [ IF EXISTS ] name ALTER [ COLUMN ] column_number | column_name
 SET STATISTICS integer
 ALTER INDEX ALL IN TABLESPACE name [ OWNED BY role_name [, ... ] ]
 SET TABLESPACE new_tablespace [ NOWAIT ]
@@ -137,14 +137,12 @@ ALTER INDEX ALL IN TABLESPACE name

 

-ALTER [ COLUMN ] column_number SET STATISTICS integer
+ALTER [ COLUMN ] column_number | column_name SET STATISTICS integer
 
  
   This form sets the per-column statistics-gathering target for
   subsequent  operations, though can
   be used only on index columns that are defined as an expression.
-  Since expressions lack a unique name, we refer to them using the
-  ordinal number of the index column.
   The target can be set in the range 0 to 1; alternatively, set it
   to -1 to revert to using the system default statistics
   target ().
@@ -175,6 +173,15 @@ ALTER INDEX ALL IN TABLESPACE name
   
  
 
+ 
+  column_name
+  
+   
+The name of an existing index column.
+   
+  
+ 
+
  
   column_number
   

Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed data

[Petr Jelinek]

Hi,

On 28/11/2018 02:14, Andres Freund wrote:
> Hi,
> 
> On 2018-11-28 02:04:18 +0100, Tomas Vondra wrote:
>>
>> On 11/24/18 12:20 AM, Tomas Vondra wrote:
>>> ...
>>>
>>> OK, here's an updated patch, tweaking the reorderbuffer part. I plan
>>> to push this sometime mid next week.
>>>
>>
>> Pushed and backpatched to 9.4- (same as e9edc1ba).
> 
> Backpatching seems on the more aggressive end of things for an
> optimization. Could you at least announce that beforehand next time?
> 

Well, it may be optimization, but from what I've seen the problems
arising from this can easily prevent logical replication from working
altogether as reorder buffer hits OOM on bigger tables. So ISTM that it
does warrant backpatch.

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

"pg_ctl: the PID file ... is empty" at end of make check

[Thomas Munro]

Hello,

Today I saw a one-off case of $SUBJECT, on macOS.  I can't reproduce
it, but I noticed exactly the same thing on longfin the other day:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2018-11-25%2005%3A39%3A04

Anyone know what that's about?

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

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Michael Paquier]

On Tue, Nov 27, 2018 at 08:17:12PM -0500, Stephen Frost wrote:
> * Michael Paquier (mich...@paquier.xyz) wrote:
>> Please see 0002 attached, which moves the call to skipfile() where I
>> think it should go.
> 
> Alright, on a quick glance that seems ok.

Thanks.

>> Base backups are impacted as well as this causes spurious warnings.
> 
> Right- could you please also add comments around the two lists to make
> other hackers aware that the list shows up in two places and that any
> changes should be made in both places..?

Good point.  Added in my local branch.

>> Attached are two patches to fix all the mess:
>> - 0001 is a revert of the whitelist, minus the set of regression tests
>> checking after corrupted files and empty files.
>> - 0002 is a fix for all the issues reported on this thread, with tests
>> added (including the tablespace test from Michael Banck):
>> -- Base backups gain EXEC_BACKEND files in their warning filters.
>> -- pg_verify_checksums gains the same files.
>> -- temporary files are filtered out.
>> -- pg_verify_checksums performs filtering checks only on regular files,
>> not on paths.
>> 
>> 0001 and 0002 need to be merged as 0001 would cause the buildfarm to
>> turn red on Windows if applied alone.  Can you know see my point?
> 
> Yes, I think they could be merged to address that, though I'm not sure
> that it's necessairly a huge deal either, if they're going to be pushed
> together.

This avoids noise failures when bisecting a regression, which matters in
some cases.  To keep the history cleaner perhaps you are right and it
would be cleaner to split into two commits.

Let's wait a bit and see if others have extra opinions to offer.
--
Michael


signature.asc
Description: PGP signature

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Nov 27, 2018 at 06:27:57PM -0500, Stephen Frost wrote:
> > * Michael Paquier (mich...@paquier.xyz) wrote:
> >> Believe me or not, but we have spent so much energy on this stuff that I
> >> am ready to give up on the whitelist patch and focus on other business.
> > 
> > I would have hoped that you'd see why I was concerned from the start
> > about this now that we have a released version of pg_verify_checksums
> > in 11.1 that is paper-bag-worthy.
> 
> That's unrelated.  Let's be clear, I still don't like the fact that we
> don't use a whitelist approach, per the arguments raised upthread.  The
> tool also clearly lacked testing and coverage from the beginning and has
> never been able to work on Windows, which was a very bad decision.
> Still we need to live with that and I take my share of the blame,
> willing to make this stuff work better for all.

I don't agree that it's unrelated and I'm disappointed that you feel it
is, but I'm not going to belabor the point.

> >> - skipfile should be called after making sure that we work on a file.
> > 
> > It's not clear to me what this is referring to, exactly...?  Can you
> > clarify?
> 
> Please see 0002 attached, which moves the call to skipfile() where I
> think it should go.

Alright, on a quick glance that seems ok.

> >> - it is necessary to exclude EXEC_BACKEND files.
> > 
> > Agreed, they should be added to the exclude list.
> 
> Base backups are impacted as well as this causes spurious warnings.

Right- could you please also add comments around the two lists to make
other hackers aware that the list shows up in two places and that any
changes should be made in both places..?

> Attached are two patches to fix all the mess:
> - 0001 is a revert of the whitelist, minus the set of regression tests
> checking after corrupted files and empty files.
> - 0002 is a fix for all the issues reported on this thread, with tests
> added (including the tablespace test from Michael Banck):
> -- Base backups gain EXEC_BACKEND files in their warning filters.
> -- pg_verify_checksums gains the same files.
> -- temporary files are filtered out.
> -- pg_verify_checksums performs filtering checks only on regular files,
> not on paths.
> 
> 0001 and 0002 need to be merged as 0001 would cause the buildfarm to
> turn red on Windows if applied alone.  Can you know see my point?

Yes, I think they could be merged to address that, though I'm not sure
that it's necessairly a huge deal either, if they're going to be pushed
together.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed data

[Andres Freund]

Hi,

On 2018-11-28 02:04:18 +0100, Tomas Vondra wrote:
> 
> On 11/24/18 12:20 AM, Tomas Vondra wrote:
> > ...
> > 
> > OK, here's an updated patch, tweaking the reorderbuffer part. I plan
> > to push this sometime mid next week.
> > 
> 
> Pushed and backpatched to 9.4- (same as e9edc1ba).

Backpatching seems on the more aggressive end of things for an
optimization. Could you at least announce that beforehand next time?

Greetings,

Andres Freund

Re: Function to promote standby servers

[Michael Paquier]

On Wed, Nov 28, 2018 at 10:06:34AM +0900, Ian Barwick wrote:
> Thanks, looks good (and apologies for the delay in responding from my
> side).

Thanks for double-checking, Ian.  I took my time as well ;)

(Hope to see you face-to-face in a couple of days around Akihabara.)
--
Michael


signature.asc
Description: PGP signature

Re: Function to promote standby servers

[Ian Barwick]

On 11/19/2018 01:22 PM, Michael Paquier wrote:

On Fri, Oct 26, 2018 at 01:51:24PM +0900, Michael Paquier wrote:

Or we could use "the function returns true immediately after initiating
the promotion by sending the promotion signal to the postmaster"?  As a
native speaker which one feels more natural to you?


Sorry for the time it took.  After pondering on it, I have committed the
improvements from your version.


Thanks, looks good (and apologies for the delay in responding from my
side).


Regards


Ian Barwick


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

Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed data

[Tomas Vondra]

On 11/24/18 12:20 AM, Tomas Vondra wrote:
> ...
> 
> OK, here's an updated patch, tweaking the reorderbuffer part. I plan
> to push this sometime mid next week.
> 

Pushed and backpatched to 9.4- (same as e9edc1ba).

regards

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

Re: MERGE SQL statement for PG12

[Amit Langote]

Hi Pavan,

Thanks for continuing to work on this.

On 2018/11/27 20:18, Pavan Deolasee wrote:
> Ok. I will try that approach again. In the meanwhile, I am posting a
> rebased version. There had been quite a lot changes on partitioning side
> and that caused non-trivial conflicts. I noticed a couple of problems
> during the rebase, but I haven't attempted to address them fully yet, since
> I think a detailed review at some point might require us to change that
> anyways.
> 
> - Noticed that partColsUpdated is not set correctly in case of MERGE since
> we never get to call expand_partitioned_rtentry() for the partition table
> in case of MERGE. This right now does not cause significant problem, since
> we initialise the required states by other means. But we should fix this.

Regarding this, you may want to take a look at the following patch that
refactors things in this area.

https://commitfest.postgresql.org/20/1778/

Among other changes (such as completely redesigned inheritance_planner),
expand_partitioned_rtentry is now called after entering make_one_rel()
with the patch, which is much later than currently.  That allows us to
initialize RTEs and RelOptInfos for only the partitions that are left
after pruning.  As of now, it's called at a point in subquery_planner
where it's too soon to prune, so  expand_partitioned_rtentry ends up
building RTEs for *all* partitions.  I think that is something we're going
to have change in the long run anyway, so perhaps something you should
consider when designing related parts of MERGE.  I will try to look at
that portion of your patch maybe next month.

Thanks,
Amit

Re: Minor typo

[Michael Paquier]

On Tue, Nov 27, 2018 at 07:51:03PM -0500, Stephen Frost wrote:
> That isn't at all what I got from that.
> 
> A rewrite of this really should avoid talking about removing the hole as
> if it's 'compression' because, first of all, it isn't, and second, now
> that we have *actual* compression happening, it's terribly confusing to
> talk about removing the hole using that terminology.

You may want to be careful about other comments in xloginsert.c or such
then.  Removing a page hole is mentioned in a couple of places as a form
of compression if I recall correctly.
--
Michael


signature.asc
Description: PGP signature

Re: Handling of REGRESS_OPTS in MSVC for regression tests

[Michael Paquier]

On Tue, Nov 27, 2018 at 05:59:34PM -0500, Andrew Dunstan wrote:
> I think you should just proceed with the changes above. I just had a quick
> look at the patch you posted before, and it looks sane enough.

Thanks for the feedback, Andrew.  Let's wait a couple of days and see if
anybody has any objection about the patch set.

> I don't see a need to backpatch.

Okay, no problem with that.
--
Michael


signature.asc
Description: PGP signature

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Michael Paquier]

On Tue, Nov 27, 2018 at 06:27:57PM -0500, Stephen Frost wrote:
> * Michael Paquier (mich...@paquier.xyz) wrote:
>> Believe me or not, but we have spent so much energy on this stuff that I
>> am ready to give up on the whitelist patch and focus on other business.
> 
> I would have hoped that you'd see why I was concerned from the start
> about this now that we have a released version of pg_verify_checksums
> in 11.1 that is paper-bag-worthy.

That's unrelated.  Let's be clear, I still don't like the fact that we
don't use a whitelist approach, per the arguments raised upthread.  The
tool also clearly lacked testing and coverage from the beginning and has
never been able to work on Windows, which was a very bad decision.
Still we need to live with that and I take my share of the blame,
willing to make this stuff work better for all.

>> - skipfile should be called after making sure that we work on a file.
> 
> It's not clear to me what this is referring to, exactly...?  Can you
> clarify?

Please see 0002 attached, which moves the call to skipfile() where I
think it should go.

>> - it is necessary to exclude EXEC_BACKEND files.
> 
> Agreed, they should be added to the exclude list.

Base backups are impacted as well as this causes spurious warnings.

Attached are two patches to fix all the mess:
- 0001 is a revert of the whitelist, minus the set of regression tests
checking after corrupted files and empty files.
- 0002 is a fix for all the issues reported on this thread, with tests
added (including the tablespace test from Michael Banck):
-- Base backups gain EXEC_BACKEND files in their warning filters.
-- pg_verify_checksums gains the same files.
-- temporary files are filtered out.
-- pg_verify_checksums performs filtering checks only on regular files,
not on paths.

0001 and 0002 need to be merged as 0001 would cause the buildfarm to
turn red on Windows if applied alone.  Can you know see my point?
--
Michael
From ad07e7c4ef9c40e2fc13ff59bb51a071ab69ebbe Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 28 Nov 2018 09:32:44 +0900
Subject: [PATCH 1/2] Switch pg_verify_checksums back to a blacklist

This basically reverts commit d55241af705667d4503638e3f77d3689fd6be31,
leaving around a portion of the regression tests still adapted with
empty relation files, and corrupted cases.
---
 .../pg_verify_checksums/pg_verify_checksums.c | 77 ---
 src/bin/pg_verify_checksums/t/002_actions.pl  | 17 
 2 files changed, 17 insertions(+), 77 deletions(-)

diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index f0e09bea20..1bc020ab6c 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -15,7 +15,6 @@
 
 #include "catalog/pg_control.h"
 #include "common/controldata_utils.h"
-#include "common/relpath.h"
 #include "getopt_long.h"
 #include "pg_getopt.h"
 #include "storage/bufpage.h"
@@ -50,69 +49,27 @@ usage(void)
 	printf(_("Report bugs to .\n"));
 }
 
-/*
- * isRelFileName
- *
- * Check if the given file name is authorized for checksum verification.
- */
+static const char *const skip[] = {
+	"pg_control",
+	"pg_filenode.map",
+	"pg_internal.init",
+	"PG_VERSION",
+	NULL,
+};
+
 static bool
-isRelFileName(const char *fn)
+skipfile(const char *fn)
 {
-	int			pos;
+	const char *const *f;
 
-	/*--
-	 * Only files including data checksums are authorized for verification.
-	 * This is guessed based on the file name by reverse-engineering
-	 * GetRelationPath() so make sure to update both code paths if any
-	 * updates are done.  The following file name formats are allowed:
-	 * 
-	 * .
-	 * _
-	 * _.
-	 *
-	 * Note that temporary files, beginning with 't', are also skipped.
-	 *
-	 *--
-	 */
-
-	/* A non-empty string of digits should follow */
-	for (pos = 0; isdigit((unsigned char) fn[pos]); ++pos)
-		;
-	/* leave if no digits */
-	if (pos == 0)
-		return false;
-	/* good to go if only digits */
-	if (fn[pos] == '\0')
+	if (strcmp(fn, ".") == 0 ||
+		strcmp(fn, "..") == 0)
 		return true;
 
-	/* Authorized fork files can be scanned */
-	if (fn[pos] == '_')
-	{
-		int			forkchar = forkname_chars(&fn[pos + 1], NULL);
-
-		if (forkchar <= 0)
-			return false;
-
-		pos += forkchar + 1;
-	}
-
-	/* Check for an optional segment number */
-	if (fn[pos] == '.')
-	{
-		int			segchar;
-
-		for (segchar = 1; isdigit((unsigned char) fn[pos + segchar]); ++segchar)
-			;
-
-		if (segchar <= 1)
-			return false;
-		pos += segchar;
-	}
-
-	/* Now this should be the end */
-	if (fn[pos] != '\0')
-		return false;
-	return true;
+	for (f = skip; *f; f++)
+		if (strcmp(*f, fn) == 0)
+			return true;
+	return false;
 }
 
 static void
@@ -189,7 +146,7 @@ scan_directory(const char *basedir, const char *subdir)
 		char		fn[MAXPGPATH];
 		struct stat st;
 
-		if (!isRelFileName(de->d_name))
+		if (skipfile(de->d_name))
 			continue;
 
 		snprintf(fn, sizeof(fn), "%s/%s", path

Re: Minor typo

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Nov 27, 2018 at 07:18:39PM -0500, Tom Lane wrote:
> >   * When wal_compression is enabled and a "hole" is removed from a full
> >   * page image, the page image is compressed using PGLZ compression.
> > 
> > (BTW, is this trying to say that we don't apply compression if the page
> > contains no hole?  If so, why not?)
> 
> Compression can be applied on a page with or without a hole.  What this
> sentence means is that the equivalent of a double level of compression
> can be applied on top of the hole being removed, if the hole can be
> removed.

That isn't at all what I got from that.

A rewrite of this really should avoid talking about removing the hole as
if it's 'compression' because, first of all, it isn't, and second, now
that we have *actual* compression happening, it's terribly confusing to
talk about removing the hole using that terminology.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Minor typo

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Daniel Gustafsson  writes:
> >> On 28 Nov 2018, at 00:43, Stephen Frost  wrote:
> >> Sending it here in case anyone feels that we should do more than just
> >> correct the word..?  Perhaps for non-native English speakers seeing
> >> "whose" used here is confusing?
> 
> > Being a non-native English speaker I think it’s fine and, in my own bias,
> > commonly understood.
> 
> I agree that "which" is just wrong here, but "whose" seems a bit malaprop
> as well, and it's not the only poor English in that sentence.  Maybe
> rewrite the whole sentence to avoid the problem?
> 
>   * When wal_compression is enabled and a "hole" is removed from a full
>   * page image, the page image is compressed using PGLZ compression.

Yeah, that seems a bit cleaner.

> (BTW, is this trying to say that we don't apply compression if the page
> contains no hole?  If so, why not?)

Sure seems to be saying that, and later on..

 * If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED, an
 * XLogRecordBlockCompressHeader struct follows.

Yet XLogCompressBackupBlock() sure looks like it'll happily compress a
page even if it doesn't have a hole.  The replay logic certainly seems
to only check if BKPIMAGE_IS_COMPRESSED is set.

I'm thinking there's quite a bit of room for improvement here...  I
wonder if the idea originally was "the page is 'compressed', in some
way, if it either had a hole or was actually compressed"..

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Minor typo

[Michael Paquier]

On Tue, Nov 27, 2018 at 07:18:39PM -0500, Tom Lane wrote:
>   * When wal_compression is enabled and a "hole" is removed from a full
>   * page image, the page image is compressed using PGLZ compression.
> 
> (BTW, is this trying to say that we don't apply compression if the page
> contains no hole?  If so, why not?)

Compression can be applied on a page with or without a hole.  What this
sentence means is that the equivalent of a double level of compression
can be applied on top of the hole being removed, if the hole can be
removed.
--
Michael


signature.asc
Description: PGP signature

Re: Minor typo

[Tom Lane]

Daniel Gustafsson  writes:
>> On 28 Nov 2018, at 00:43, Stephen Frost  wrote:
>> Sending it here in case anyone feels that we should do more than just
>> correct the word..?  Perhaps for non-native English speakers seeing
>> "whose" used here is confusing?

> Being a non-native English speaker I think it’s fine and, in my own bias,
> commonly understood.

I agree that "which" is just wrong here, but "whose" seems a bit malaprop
as well, and it's not the only poor English in that sentence.  Maybe
rewrite the whole sentence to avoid the problem?

  * When wal_compression is enabled and a "hole" is removed from a full
  * page image, the page image is compressed using PGLZ compression.

(BTW, is this trying to say that we don't apply compression if the page
contains no hole?  If so, why not?)

regards, tom lane

Re: [PATCH] Tiny CREATE STATISTICS tab-completion cleanup

[Tomas Vondra]

On 11/27/18 12:55 AM, Tomas Vondra wrote:
> Hi,
> 
> On 11/26/18 5:49 PM, Dagfinn Ilmari Mannsåker wrote:
>> Hi Hackers,
>>
>> As I was hacking on the CREATE TABLE tab completions, I noticed that the
>> CREATE STATISTICS completion was checking manually for the start and end
>> of the parenthesised list instead of using the "(*)" wildcard (because
>> it predates that functionality).  Attached is a patch that updates it to
>> use the modern syntax.
>>
> 
> Makes sense. At first I was wondering why it was not modified by the
> patch introducing the "(*)" wildcard, but I see 121213d9 only cared
> about VACUUM, EXPLAIN and ANALYZE.
> 
> The patch seems fine to me, I'll get it committed.
> 

Pushed. Thanks for the patch.


regards

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

Re: Minor typo

[Daniel Gustafsson]

> On 28 Nov 2018, at 00:43, Stephen Frost  wrote:

> Sending it here in case anyone feels that we should do more than just
> correct the word..?  Perhaps for non-native English speakers seeing
> "whose" used here is confusing?

Being a non-native English speaker I think it’s fine and, in my own bias,
commonly understood.

+1 on raising the “does this make sense for a non-native speaker” question
though, making the documentation (regardless of if it’s docs, code comments or
READMEs) accessible is very important.

cheers ./daniel

Minor typo

[Stephen Frost]

Greetings,

While reviewing a bit of code around full page images, I came across a
typo and fixed it in the attach.

Sending it here in case anyone feels that we should do more than just
correct the word..?  Perhaps for non-native English speakers seeing
"whose" used here is confusing?

If I don't hear any concerns then I'll go ahead and push it some time
tomorrow; it's a pretty minor change, of course, and we can always
adjust it further if someone raises an issue later too.

Thanks!

Stephen
From fb9b0f00396179af895c13e44bff98eee8302894 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Tue, 27 Nov 2018 18:37:59 -0500
Subject: [PATCH] Fix typo

Change:

When wal_compression is enabled, a full page image which "hole" was
removed is additionally compressed using PGLZ compression algorithm.

to:

When wal_compression is enabled, a full page image whose "hole" was
removed is additionally compressed using PGLZ compression algorithm.

As the correct word to use here is "whose", not "which".
---
 src/include/access/xlogrecord.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h
index 863781937e..ccf92def93 100644
--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -114,7 +114,7 @@ typedef struct XLogRecordBlockHeader
  * XLOG record's CRC, either).  Hence, the amount of block data actually
  * present is BLCKSZ - the length of "hole" bytes.
  *
- * When wal_compression is enabled, a full page image which "hole" was
+ * When wal_compression is enabled, a full page image whose "hole" was
  * removed is additionally compressed using PGLZ compression algorithm.
  * This can reduce the WAL volume, but at some extra cost of CPU spent
  * on the compression during WAL logging. In this case, since the "hole"
-- 
2.17.1



signature.asc
Description: PGP signature

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Nov 27, 2018 at 05:45:41PM -0500, Stephen Frost wrote:
> > This doesn't exactly change my opinion regarding this discussion and I'd
> > rather we revert the "whitelist" patch and use the very minimal patch
> > from here:
> > 
> > https://www.postgresql.org/message-id/20181012013918.GA30064%40paquier.xyz
> 
> Believe me or not, but we have spent so much energy on this stuff that I
> am ready to give up on the whitelist patch and focus on other business.

I would have hoped that you'd see why I was concerned from the start
about this now that we have a released version of pg_verify_checksums
in 11.1 that is paper-bag-worthy.

> This doesn't change a couple of things though, so it is not *just* a
> simple revert with the patch you mention applied:
> - Adding a test for tablespaces makes sense.

I agree with this, of course.

> - skipfile should be called after making sure that we work on a file.

It's not clear to me what this is referring to, exactly...?  Can you
clarify?

> - temporary files and temporary paths should be ignored.

There's example code for how to do this in basebackup.c

> - it is necessary to exclude EXEC_BACKEND files.

Agreed, they should be added to the exclude list.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Accounting of zero-filled buffers in EXPLAIN (BUFFERS)

[Thomas Munro]

On Tue, Nov 27, 2018 at 2:53 PM Haribabu Kommi  wrote:
> Thanks for the updated patch. It looks good.
> I marked it in the commitfest as ready for committer.

Pushed.  Thanks for the reviews!

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

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Michael Paquier]

On Tue, Nov 27, 2018 at 05:45:41PM -0500, Stephen Frost wrote:
> This doesn't exactly change my opinion regarding this discussion and I'd
> rather we revert the "whitelist" patch and use the very minimal patch
> from here:
> 
> https://www.postgresql.org/message-id/20181012013918.GA30064%40paquier.xyz

Believe me or not, but we have spent so much energy on this stuff that I
am ready to give up on the whitelist patch and focus on other business.
This doesn't change a couple of things though, so it is not *just* a
simple revert with the patch you mention applied:
- Adding a test for tablespaces makes sense.
- skipfile should be called after making sure that we work on a file.
- temporary files and temporary paths should be ignored.
- it is necessary to exclude EXEC_BACKEND files.
--
Michael


signature.asc
Description: PGP signature

Re: Handling of REGRESS_OPTS in MSVC for regression tests

[Andrew Dunstan]

On 11/27/18 4:10 PM, Michael Paquier wrote:

On Tue, Nov 27, 2018 at 10:27:17AM +0900, Michael Paquier wrote:

Okay, let's do so by supporting correctly NO_INSTALLCHECK.  My other
refactoring work can also live with that.  Returning an empty list via
fetchTests() and bypass a run if nothing is present looks fine to me.
One extra thing is that modules/commit_ts and modules/test_rls_hooks are
missing NO_INSTALLCHECK, so we would need to add that part to be
completely correct.  I would also be inclined to back-patch both parts,
however for my stuff I could live with this patch only on HEAD, and
anybody willing to use installcheck on commit_ts and test_rls_hooks may
be surprised to not be able to do that anymore after the minor release.
It still looks incorrect to me though to be able to run installcheck on
those modules though...

Attached is a proposal of patch, which works as I would expect with
modulescheck and contribcheck.  How does that look?

If possible, I would like to move on with this stuff.  To keep things
green in the buildfarm all the time, I would like to do that with two
independent steps:
1) Add NO_INSTALLCHECK to modules/commit_ts and modules/test_rls_hook.
2) Add support for NO_INSTALLCHECK in the MSVC scripts.

Are there any objections?  I could do a back-patch as well to keep
things consistent across branches if there are voices in favor of that,
but that's not necessary for the upcoming Makefile cleanup with the new
set of PGXS options.



I think you should just proceed with the changes above. I just had a 
quick look at the patch you posted before, and it looks sane enough.



I don't see a need to backpatch.


cheers


andrew

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

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Mon, Nov 19, 2018 at 10:17:19PM -0500, Stephen Frost wrote:
> > Let's try to not conflate these two issues though, they're quite
> > independent.
> 
> This is a poke about a recent issue raised by Michael Banck here:
> https://www.postgresql.org/message-id/f1543332405.17247.9.ca...@credativ.de
> And for which I am suggesting a minimal fix, which is extracted from a
> patch at the most-top of this thread:
> https://www.postgresql.org/message-id/20181127213625.gm1...@paquier.xyz
> 
> If there are no objections, I would like to fix the actual issue.  Then
> I'll rebase a patch on top of what has been fixed for this thread for
> what I proposed in the base backup code.

So the as-committed "whitelist" approach did, in fact, end up excluding
huge portions of the database from being checked, that is, everything
inside of tablespaces.

This doesn't exactly change my opinion regarding this discussion and I'd
rather we revert the "whitelist" patch and use the very minimal patch
from here:

https://www.postgresql.org/message-id/20181012013918.GA30064%40paquier.xyz

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Nov 27, 2018 at 09:45:04AM -0500, Stephen Frost wrote:
> > If you don't consider your recovery scripts and your backup scripts to
> > be related then I've really got to wonder how you're regularly testing
> > your backups to make sure that they're actually valid.
> 
> Base backups can be perfectly self-contained as long as they include all
> the WAL segments needed to recover up to the end-of-backup record.
> That's what pg_basebackup does with its default options
> (--wal-method=stream in particular).

This really doesn't change my opinion at all.  Sure, some custom scripts
might have been written to operate in this way and now they'll need to
be adjusted to work the exact same way pg_basebackup works today and use
the non-exclusive mode, but they'll be better off for it and won't have
the concern about what happens if the system is inadvertantly restarted
during a backup.

I'd also say it's likely that places which know enough to build such a
solution aren't ones that we really need to worry about having an issue
adjusting their scripts.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[David Steele]

On 11/27/18 4:25 PM, Michael Paquier wrote:
> On Tue, Nov 27, 2018 at 09:45:04AM -0500, Stephen Frost wrote:
>> If you don't consider your recovery scripts and your backup scripts to
>> be related then I've really got to wonder how you're regularly testing
>> your backups to make sure that they're actually valid.
> 
> Base backups can be perfectly self-contained as long as they include all
> the WAL segments needed to recover up to the end-of-backup record.
> That's what pg_basebackup does with its default options
> (--wal-method=stream in particular).

This is true, of course, but it misses one of the major benefits of
file-level backups which is PITR.

>> If you aren't regularly testing your backups then I've got little
>> sympathy.
> 
> Fortunately they do, hundreds of time on a daily basis ;)

Nice!

>> To be clear, pgbackrest doesn't have any dependency here- but it, like
>> all of the other 3rd party backup solutions and any restore solution
>> that a user has come up with, are going to have to be changed to deal
>> with the changes in how recovery works, so this is a good time to make
>> these changes.
> 
> My point is that base backups do not have a mandatory dependency with
> recovery.conf all the time as they can perfectly be restored if they are
> standalone backups.  I can see a dependency with recovery.conf once you
> have a base backup which needs to be fed with WAL segments from an
> external archive, or when using a base backup to create a standby.

If you want to do PITR -- which is the default in most situations --
then some interaction with recovery.conf is needed.  I think you would
be hard-pressed to find a prominent HA or backup solution that doesn't
do so.

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



signature.asc
Description: OpenPGP digital signature

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Michael Paquier]

On Tue, Nov 27, 2018 at 09:49:29PM +, Bossart, Nathan wrote:
> That sounds good to me.  I was actually thinking of using the same
> retry counter that we use for pgarch_archiveXlog(), but on second
> thought, it is probably better to have two independent retry counters
> for these two unrelated operations.

What I had in mind was two different variables if what I wrote was
unclear, possibly with the same value, as archiving failure and failure
with orphan file removals are two different concepts.
--
Michael


signature.asc
Description: PGP signature

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Bossart, Nathan]

On 11/27/18, 3:20 PM, "Michael Paquier"  wrote:
> On Tue, Nov 27, 2018 at 08:43:06PM +, Bossart, Nathan wrote:
>> IIUC any time that the file does not exist, we will attempt to unlink
>> it.  Regardless of whether unlinking fails or succeeds, we then
>> proceed to give up archiving for now, but it's not clear why.  Perhaps
>> we should retry unlinking a number of times (like we do for
>> pgarch_archiveXlog()) when durable_unlink() fails and simply "break"
>> to move on to the next .ready file if durable_unlink() succeeds.
>
> Both suggestions sound reasonable to me.  (durable_unlink is not called
> on HEAD in pgarch_archiveXlog).  How about 3 retries with a in-between
> wait of 1s?  That's consistent with what pgarch_ArchiverCopyLoop does,
> still I am not completely sure if we actually want to be consistent for
> the purpose of removing orphaned ready files.

That sounds good to me.  I was actually thinking of using the same
retry counter that we use for pgarch_archiveXlog(), but on second
thought, it is probably better to have two independent retry counters
for these two unrelated operations.

Nathan

Re: pg11b1 from outside a txn: "VACUUM cannot run inside a transaction block": should be: ...or multi-command string

[Justin Pryzby]

I'm resending a mail from June:
https://www.postgresql.org/message-id/flat/87sh5doya9.fsf%40news-spur.riddles.org.uk#83c3d1a183217204939252d56804f247

This is maybe a trivial change in ERROR string which maybe worth changing.

On PG10:
|[pryzbyj@database ~]$ psql postgres -c 'DROP DATABASE x; CREATE DATABASE x';
|ERROR:  DROP DATABASE cannot be executed from a function or multi-command 
string

On PG11.1:
|[pryzbyj@telsasoft2015 server]$ psql postgres -c 'DROP DATABASE x; CREATE 
DATABASE x';
|ERROR:  DROP DATABASE cannot run inside a transaction block

I spent at least a few minutes trying to do things like: psql -c 
'begin;commit;create;drop'
..before figuring it out due to misleading error message, despite having
written this earlier message.

On Sat, Jun 23, 2018 at 04:06:37PM -0500, Justin Pryzby wrote:
> in pg10:
> 
>   ts=# begin;VACUUM FULL pg_toast.pg_toast_2619;
>   BEGIN
>   ERROR:  25001: VACUUM cannot run inside a transaction block
>   LOCATION:  PreventTransactionChain, xact.c:3167
> => sounds fine
> 
>   $ psql postgres -c 'SELECT 1; VACUUM pg_statistic'
>   ERROR:  VACUUM cannot be executed from a function or multi-command 
> string
> => sounds fine
> 
> In pg11b1:
> 
>   pryzbyj=# begin;VACUUM FULL pg_toast.pg_toast_2619;
>   BEGIN
>   ERROR:  25001: VACUUM cannot run inside a transaction block
>   LOCATION:  PreventInTransactionBlock, xact.c:3163
> => sounds fine
> 
>   [pryzbyj@dev ~]$ psql -c 'SELECT 1; VACUUM pg_statistic'
>   ERROR:  VACUUM cannot run inside a transaction block
> => Error message seems off??
> 
> I couldn't figure out how to \set VERBOSITY verbose inside a psql command 
> (??),
> but strace shows for v10:
> SERROR\0VERROR\0C25001\0MVACUUM cannot be executed from a function or 
> multi-command string\0Fxact.c\0L3187\0RPreventTransactionChain
> 
> And for v11:
> SERROR\0VERROR\0C25001\0MVACUUM cannot run inside a transaction 
> block\0Fxact.c\0L3163\0RPreventInTransactionBlock
> 
> Function renamed by commit 04700b685f31508036456bea4d92533e5ceee9d6.
> 
> So behavior change maybe caused by 6eb52da3948dc8bc7c8a61cbacac14823b670c58 ?
> 
> Justin

Re: More issues with pg_verify_checksums and checksum verification in base backups

[Michael Paquier]

On Mon, Nov 19, 2018 at 10:17:19PM -0500, Stephen Frost wrote:
> Let's try to not conflate these two issues though, they're quite
> independent.

This is a poke about a recent issue raised by Michael Banck here:
https://www.postgresql.org/message-id/f1543332405.17247.9.ca...@credativ.de
And for which I am suggesting a minimal fix, which is extracted from a
patch at the most-top of this thread:
https://www.postgresql.org/message-id/20181127213625.gm1...@paquier.xyz

If there are no objections, I would like to fix the actual issue.  Then
I'll rebase a patch on top of what has been fixed for this thread for
what I proposed in the base backup code.
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Add TAP tests for pg_verify_checksums

[Michael Paquier]

On Tue, Nov 27, 2018 at 04:26:45PM +0100, Michael Banck wrote:
> Oh, I kinda followed that thread a bit, but I think that patch fixes
> things more by matter of moving code around, at least I haven't noticed
> that tablespaces were explicitly claimed to be fixed in that thread.

That may have been an unconscious move ;)
It seems to me the root issue is that the original implementation of
pg_verify_checksums naively assumed that files can be skipped without
first checking at their types, which I got confused by in d55241af.

You should definitely be mentioned as this reporter anyway, and get
author credits for the additional test.

> As pg_verify_checksums appears to be broken with respect to tablespaces
> in REL_11_STABLE (so I think 11.1, but not 11.0) as well, I think this
> merits a short-term minimal invasive fix (i.e. the patch you posted,
> just that there's no TAP testsuite for pg_verify_checksums in v11
> unfortunately) on its own, regardless of the wider changes proposed in
> the other thread.

Yes, let's do so rather sooner than later if there are no objections
from others.  I am going to ping the other thread about what's discussed
here additionally in case folks missed what you reported.  Fixing the
temp file issue which has been reported by Andres first is an additional
bonus.
--
Michael


signature.asc
Description: PGP signature

Re: tab-completion debug print

[David Fetter]

On Tue, Nov 27, 2018 at 03:54:55PM -0500, Tom Lane wrote:
> David Fetter  writes:
> > Do we still want this as a compile-time option, or does it make more
> > sense as a run-time option? I'm thinking that with \L, it might make
> > sense as a run-time option.
> 
> This seems to me to be strictly a developer debugging feature.

I would have thought so, but as our tab completion gets bigger--and
it's been steadily doing that--we may find ourselves in a situation
where it'd be handy to debug things in a production environment, with
all the restrictions that implies.

Can we conceive of a circumstance where the check for -L/\L would be
significant?  I've seen people type pretty quickly, but not thus far
fast enough to notice a cache miss.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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

Re: Remove Deprecated Exclusive Backup Mode

[Michael Paquier]

On Tue, Nov 27, 2018 at 09:45:04AM -0500, Stephen Frost wrote:
> If you don't consider your recovery scripts and your backup scripts to
> be related then I've really got to wonder how you're regularly testing
> your backups to make sure that they're actually valid.

Base backups can be perfectly self-contained as long as they include all
the WAL segments needed to recover up to the end-of-backup record.
That's what pg_basebackup does with its default options
(--wal-method=stream in particular).

> If you aren't regularly testing your backups then I've got little
> sympathy.

Fortunately they do, hundreds of time on a daily basis ;)

> To be clear, pgbackrest doesn't have any dependency here- but it, like
> all of the other 3rd party backup solutions and any restore solution
> that a user has come up with, are going to have to be changed to deal
> with the changes in how recovery works, so this is a good time to make
> these changes.

My point is that base backups do not have a mandatory dependency with
recovery.conf all the time as they can perfectly be restored if they are
standalone backups.  I can see a dependency with recovery.conf once you
have a base backup which needs to be fed with WAL segments from an
external archive, or when using a base backup to create a standby.
--
Michael


signature.asc
Description: PGP signature

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Michael Paquier]

On Tue, Nov 27, 2018 at 08:43:06PM +, Bossart, Nathan wrote:
> Don't we also need to check that errno is ENOENT here?

Yep.

> IIUC any time that the file does not exist, we will attempt to unlink
> it.  Regardless of whether unlinking fails or succeeds, we then
> proceed to give up archiving for now, but it's not clear why.  Perhaps
> we should retry unlinking a number of times (like we do for
> pgarch_archiveXlog()) when durable_unlink() fails and simply "break"
> to move on to the next .ready file if durable_unlink() succeeds.

Both suggestions sound reasonable to me.  (durable_unlink is not called
on HEAD in pgarch_archiveXlog).  How about 3 retries with a in-between
wait of 1s?  That's consistent with what pgarch_ArchiverCopyLoop does,
still I am not completely sure if we actually want to be consistent for
the purpose of removing orphaned ready files.
--
Michael


signature.asc
Description: PGP signature

Re: Handling of REGRESS_OPTS in MSVC for regression tests

[Michael Paquier]

On Tue, Nov 27, 2018 at 10:27:17AM +0900, Michael Paquier wrote:
> Okay, let's do so by supporting correctly NO_INSTALLCHECK.  My other
> refactoring work can also live with that.  Returning an empty list via
> fetchTests() and bypass a run if nothing is present looks fine to me.
> One extra thing is that modules/commit_ts and modules/test_rls_hooks are
> missing NO_INSTALLCHECK, so we would need to add that part to be
> completely correct.  I would also be inclined to back-patch both parts,
> however for my stuff I could live with this patch only on HEAD, and
> anybody willing to use installcheck on commit_ts and test_rls_hooks may
> be surprised to not be able to do that anymore after the minor release.
> It still looks incorrect to me though to be able to run installcheck on
> those modules though...
> 
> Attached is a proposal of patch, which works as I would expect with
> modulescheck and contribcheck.  How does that look?

If possible, I would like to move on with this stuff.  To keep things
green in the buildfarm all the time, I would like to do that with two
independent steps:
1) Add NO_INSTALLCHECK to modules/commit_ts and modules/test_rls_hook.
2) Add support for NO_INSTALLCHECK in the MSVC scripts.

Are there any objections?  I could do a back-patch as well to keep
things consistent across branches if there are voices in favor of that,
but that's not necessary for the upcoming Makefile cleanup with the new
set of PGXS options.
--
Michael


signature.asc
Description: PGP signature

Re: SSL tests failing with "ee key too small" error on Debian SID

[Michael Paquier]

On Tue, Nov 27, 2018 at 09:37:17AM -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 01/10/2018 14:18, Kyotaro HORIGUCHI wrote:
>>> The attached second patch just changes key size to 2048 bits and
>>> "ee key too small" are eliminated in 001_ssltests_master, but
>>> instead I got "ca md too weak" error. This is eliminated by using
>>> sha256 instead of sha1 in cas.config. (third attached)
> 
>> I have applied these configuration changes and created a new set of test
>> files with them.
> 
> Buildfarm critters aren't going to be happy unless you back-patch that.

Thanks for applying that, Peter.
--
Michael


signature.asc
Description: PGP signature

Re: tab-completion debug print

[Tom Lane]

David Fetter  writes:
> Do we still want this as a compile-time option, or does it make more
> sense as a run-time option? I'm thinking that with \L, it might make
> sense as a run-time option.

This seems to me to be strictly a developer debugging feature.

regards, tom lane

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Bossart, Nathan]

On 11/27/18, 2:46 PM, "Andres Freund"  wrote:
> On 2018-11-27 20:43:06 +, Bossart, Nathan wrote:
>> I don't have exact figures to share, but yes, a huge number of calls
>> to sync_file_range() and fsync() can use up a lot of time.  Presumably
>> Postgres processes files individually instead of using sync() because
>> sync() may return before writing is done.  Also, sync() would affect
>> non-Postgres files.  However, it looks like Linux actually does wait
>> for writing to complete before returning from sync() [0].
>
> sync() has absolutely no way to report errors. So, we're never going to
> be able to use it.  Besides, even postgres' temp files would be a good
> reason to not use it.

Ah, I see.  Thanks for clarifying.

Nathan

Re: tab-completion debug print

[David Fetter]

On Tue, Nov 27, 2018 at 06:06:06PM +0900, Kyotaro HORIGUCHI wrote:
> Hello.
> 
> At Mon, 26 Nov 2018 07:08:53 +0100, David Fetter  wrote in 
> <20181126060853.gp...@fetter.org>
> > On Sun, Nov 25, 2018 at 11:21:51PM -0500, Tom Lane wrote:
> > > Kyotaro HORIGUCHI  writes:
> > > >> On Fri, Nov 23, 2018 at 04:32:31PM -0500, Tom Lane wrote:
> > > >>> Hm.  I can see the value of instrumenting tab-complete when you're 
> > > >>> trying
> > > >>> to debug why it did something, but this output format seems pretty 
> > > >>> terse
> > > >>> and unreadable.
> > > 
> > > > The reason for the minimal output was it won't disturb console so
> > > > much when stderr is not redirected. It could be richer if we
> > > > premise redirection.
> > > 
> > > It seems to me this behavior would be completely unusable if it's not
> > > redirected; I don't understand why you're even considering that as an
> > > interesting option.  libreadline is necessarily active when we're doing
> > > this, and it will get very confused (or at least produce a very confused
> > > display) if anything else is emitting text onto the active console line.
> 
> The mess here started because I forgot about -L option of psql. I
> wouldn't do that if I knew of it.

Am I understanding correctly that currently, -L specifies a separate
log that only shows the output of queries?

If we're using that log, we'll need to rethink how best to describe
the new sub-feature, and possibly rethink how -L should function.
Tom's suggestion about recording both the catalog queries and their
result set/error messages seems in scope.

Speaking of scope, do we want something like \L (analogous to \o) for
this, or would a thing like that be fundamentally distinct?

> psql -L ~/psqldebug.log postgres

Excellent!

Do we still want this as a compile-time option, or does it make more
sense as a run-time option? I'm thinking that with \L, it might make
sense as a run-time option.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Andres Freund]

Hi,

On 2018-11-27 20:43:06 +, Bossart, Nathan wrote:
> I don't have exact figures to share, but yes, a huge number of calls
> to sync_file_range() and fsync() can use up a lot of time.  Presumably
> Postgres processes files individually instead of using sync() because
> sync() may return before writing is done.  Also, sync() would affect
> non-Postgres files.  However, it looks like Linux actually does wait
> for writing to complete before returning from sync() [0].

sync() has absolutely no way to report errors. So, we're never going to
be able to use it.  Besides, even postgres' temp files would be a good
reason to not use it.

Greetings,

Andres Freund

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Bossart, Nathan]

On 11/21/18, 10:16 PM, "Michael Paquier"  wrote:
>> At Fri, 02 Nov 2018 14:47:08 +, Nathan Bossart
>>  wrote in
>> <154117002849.5569.14588306221618961668.p...@coridan.postgresql.org>:
>>> Granted, any added delay from this patch is unlikely to be noticeable
>>> unless your archiver is way behind and archive_status has a huge
>>> number of files.  However, I have seen cases where startup is stuck on
>>> other tasks like SyncDataDirectory() and RemovePgTempFiles() for a
>>> very long time, so perhaps it is worth considering.
>
> What's the scale of the pg_wal partition and the amount of time things
> were stuck?  I would imagine that the sync phase hurts the most, and a
> fast startup time for crash recovery is always important.

I don't have exact figures to share, but yes, a huge number of calls
to sync_file_range() and fsync() can use up a lot of time.  Presumably
Postgres processes files individually instead of using sync() because
sync() may return before writing is done.  Also, sync() would affect
non-Postgres files.  However, it looks like Linux actually does wait
for writing to complete before returning from sync() [0].

For RemovePgTempFiles(), the documentation above the function
indicates that skipping temp file cleanup during startup would
actually be okay because collisions with existing temp file names
should be handled by OpenTemporaryFile().  I assume this cleanup is
done during startup because there isn't a great alternative besides
offloading the work to a new background worker or something.

On 11/27/18, 6:35 AM, "Michael Paquier"  wrote:
> Attached is a patch showing shaped based on the idea of upthread.
> Thoughts?

I took a look at this patch.

+   /*
+* In the event of a system crash, archive status files 
may be
+* left behind as their removals are not durable, 
cleaning up
+* orphan entries here is the cheapest method.  So 
check that
+* the segment trying to be archived still exists.
+*/
+   snprintf(pathname, MAXPGPATH, XLOGDIR "/%s", xlog);
+   if (stat(pathname, &stat_buf) != 0)
+   {

Don't we also need to check that errno is ENOENT here?

+   StatusFilePath(xlogready, xlog, ".ready");
+   if (durable_unlink(xlogready, WARNING) == 0)
+   ereport(WARNING,
+   (errmsg("removed orphan 
archive status file %s",
+   
xlogready)));
+   return;

IIUC any time that the file does not exist, we will attempt to unlink
it.  Regardless of whether unlinking fails or succeeds, we then
proceed to give up archiving for now, but it's not clear why.  Perhaps
we should retry unlinking a number of times (like we do for
pgarch_archiveXlog()) when durable_unlink() fails and simply "break"
to move on to the next .ready file if durable_unlink() succeeds.

Nathan

[0] http://man7.org/linux/man-pages/man2/sync.2.html

jit comments typos (Re: pg11.1 jit segv)

[Justin Pryzby]

On Tue, Nov 27, 2018 at 10:24:52AM -0800, Andres Freund wrote:
> And pushed. Justin, thanks again for reporting the bug and then
> narrowing it down to a reproducible test case! Would've been much harder
> to diagnose without that.
> 
> I'll look into your comments patch in a bit.

Thanks for implementing and patching it :)

And thanks for remembering the patch, and reminding me.

Here's an updated copy with additional hunks.

Justin
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index ec4a250..83e4e05 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -1873,7 +1873,7 @@ CheckOpSlotCompatibility(ExprEvalStep *op, TupleTableSlot *slot)
 
 	/*
 	 * Should probably fixed at some point, but for now it's easier to allow
-	 * buffer and heap tuples to be used interchangably.
+	 * buffer and heap tuples to be used interchangeably.
 	 */
 	if (slot->tts_ops == &TTSOpsBufferHeapTuple &&
 		op->d.fetch.kind == &TTSOpsHeapTuple)
diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/llvmjit_deform.c
index 4111bf0..ba238f1 100644
--- a/src/backend/jit/llvm/llvmjit_deform.c
+++ b/src/backend/jit/llvm/llvmjit_deform.c
@@ -103,7 +103,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 	funcname = llvm_expand_funcname(context, "deform");
 
 	/*
-	 * Check which columns do have to exist, so we don't have to check the
+	 * Check which columns have to exist, so we don't have to check the
 	 * rows natts unnecessarily.
 	 */
 	for (attnum = 0; attnum < desc->natts; attnum++)
@@ -292,7 +292,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 	}
 
 	/*
-	 * Check if's guaranteed the all the desired attributes are available in
+	 * Check if it's guaranteed that all the desired attributes are available in
 	 * tuple. If so, we can start deforming. If not, need to make sure to
 	 * fetch the missing columns.
 	 */
@@ -377,7 +377,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 
 		/*
 		 * If this is the first attribute, slot->tts_nvalid was 0. Therefore
-		 * reset offset to 0 to, it be from a previous execution.
+		 * also reset offset to 0, it may be from a previous execution.
 		 */
 		if (attnum == 0)
 		{
@@ -407,7 +407,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 
 		/*
 		 * Check for nulls if necessary. No need to take missing attributes
-		 * into account, because in case they're present the heaptuple's natts
+		 * into account, because if they're present, the heaptuple's natts
 		 * would have indicated that a slot_getmissingattrs() is needed.
 		 */
 		if (!att->attnotnull)
@@ -494,13 +494,13 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 			(known_alignment < 0 || known_alignment != TYPEALIGN(alignto, known_alignment)))
 		{
 			/*
-			 * When accessing a varlena field we have to "peek" to see if we
+			 * When accessing a varlena field, we have to "peek" to see if we
 			 * are looking at a pad byte or the first byte of a 1-byte-header
 			 * datum.  A zero byte must be either a pad byte, or the first
-			 * byte of a correctly aligned 4-byte length word; in either case
+			 * byte of a correctly aligned 4-byte length word; in either case,
 			 * we can align safely.  A non-zero byte must be either a 1-byte
 			 * length word, or the first byte of a correctly aligned 4-byte
-			 * length word; in either case we need not align.
+			 * length word; in either case, we need not align.
 			 */
 			if (att->attlen == -1)
 			{
@@ -594,8 +594,8 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 		else if (att->attnotnull && attguaranteedalign && known_alignment >= 0)
 		{
 			/*
-			 * If the offset to the column was previously known a NOT NULL &
-			 * fixed width column guarantees that alignment is just the
+			 * If the offset to the column was previously known, a NOT NULL &
+			 * fixed-width column guarantees that alignment is just the
 			 * previous alignment plus column width.
 			 */
 			Assert(att->attlen > 0);
@@ -636,8 +636,8 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 	   LLVMBuildGEP(b, v_tts_nulls, &l_attno, 1, ""));
 
 		/*
-		 * Store datum. For byval datums copy the value, extend to Datum's
-		 * width, and store. For byref types, store pointer to data.
+		 * Store datum. For byval datums: copy the value, extend to Datum's
+		 * width, and store. For byref types: store pointer to data.
 		 */
 		if (att->attbyval)
 		{
diff --git a/src/backend/jit/llvm/llvmjit_inline.cpp b/src/backend/jit/llvm/llvmjit_inline.cpp
index b33a321..2ad29be 100644
--- a/src/backend/jit/llvm/llvmjit_inline.cpp
+++ b/src/backend/jit/llvm/llvmjit_inline.cpp
@@ -9,7 +9,7 @@
  * for an external function is found - not guaranteed! - the index will then
  * be used to judge their instruction count / inline worthiness. After doing
  * so for all external functions, all the referenced functions (and
- * p

Re: pg11.1 jit segv

[Andres Freund]

On 2018-11-27 00:26:55 -0800, Andres Freund wrote:
> Hi,
> 
> On 2018-11-26 22:56:09 -0600, Justin Pryzby wrote:
> > On Mon, Nov 26, 2018 at 07:00:35PM -0800, Andres Freund wrote:
> > > Could you check that the attached patch this also fixes your original
> > > issue? Going through the code to see if there's other occurances of
> > > this.
> > 
> > Confirmed that fixes my crash.
> 
> Thanks a lot for narrowing down your crash to something I can reproduce!
> 
> 
> Here's a more complete patch, with a testcase.
> 
> Tom, the test creates a 1100k column table (using \set ECHO none +
> gexec), but with a small row. Currently it's not dropped after the
> table, as I thought it might be worthwhile to be tested by
> pg_dump/upgrade etc too. You're probably the person most concerned with
> test runtimes, ... Any concerns about that? The table creation is
> quick*, on the order of 30ms.

And pushed. Justin, thanks again for reporting the bug and then
narrowing it down to a reproducible test case! Would've been much harder
to diagnose without that.

I'll look into your comments patch in a bit.

Greetings,

Andres Freund

Re: Python versions (was Re: RHEL 8.0 build)

[Andres Freund]

Hi,

On 2018-11-27 14:14:24 +0100, Peter Eisentraut wrote:
> On 25/11/2018 23:14, Tom Lane wrote:
> > Andres Freund  writes:
> >> On 2018-11-24 15:49:25 -0500, Tom Lane wrote:
> >>> There's been some preliminary discussion about starting to default to
> >>> python3, but given this project's inherent conservatism, I don't expect
> >>> that to happen for some years yet.  In any case, whenever we do pull
> >>> that trigger we'd surely do so only in HEAD not released branches, so
> >>> buildfarm owners will need to deal with the case for years more.
> > 
> >> Why don't we probe for python2 in addition to python by default? That
> >> ought to make RHEL 8 work, without making the switch just yet.
> > 
> > I'm unexcited about that because that *would* be expressing a version
> > preference --- and one that's on the wrong side of history.
> 
> I think it would be appropriate to probe in the order
> 
> python python3 python2
> 
> This would satisfy most scenarios that are valid under PEP 394.

ISTM we should first go for python, python2, python3 in all branches,
and then have a separate master only commit that changes over the order
to prefer python3 over 2. I don't think preferring 3 over 2 would be
appropriate for past branches, but it'll surely become more common to
run into distros without "python" installed.

Greetings,

Andres Freund

Re: Continue work on changes to recovery.conf API

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-27 15:36:59 +0100, Peter Eisentraut wrote:
> > That might be a useful facility, but it wouldn't really address the
> > pg_basebackup -R issue, because that creates settings that you don't
> > want going away in this manner.
> 
> Why is that / which are those?  It's not like it worked like that <
> 2dedf4d9.

primary_conninfo and restore_command are the main ones, and the idea is
that you'd like those to be specified even when you aren't in recovery,
so that you can have a symmetric config for when a given system does
become a replica.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Sequential UUID Generation

[Tomas Vondra]

On 11/19/18 2:08 PM, Uday Bhaskar V wrote:
I tried below function as which can be used as default to column. But 
every time we need to created 2 sequences, 1st one takes care of the 
first 8 bytes and 2nd takes care of the 2nd part of the UUID. I have not 
tested index and space utilization. I have to examine this. This might 
not be completely unique in the nature. but still trying for the best.




I got a bit bored a couple of days ago, so I wrote an extension 
generating UUIDs based on either a sequence or timestamp. See


  https://blog.2ndquadrant.com/sequential-uuid-generators/

for an explanation and some simple test results.


regards

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

Re: Continue work on changes to recovery.conf API

[Andres Freund]

Hi,

On 2018-11-27 15:36:59 +0100, Peter Eisentraut wrote:
> That might be a useful facility, but it wouldn't really address the
> pg_basebackup -R issue, because that creates settings that you don't
> want going away in this manner.

Why is that / which are those?  It's not like it worked like that <
2dedf4d9.

Greetings,

Andres Freund

Re: Remove Deprecated Exclusive Backup Mode

[Andres Freund]

Hi,

On 2018-11-26 23:04:35 -0500, Robert Haas wrote:
> It's not like the problems with exclusive backup are so serious that
> you can't work around them.  If you know which machine is your master,
> you can arrange to remove backup_label on reboot (only) on the master
> (only). Sure, a lot of people probably do this wrong, but it's
> infeasible to disallow all the things that people might use
> incorrectly without making the system useless.

That doesn't protect against postgres, not machine, [crash-]restarts
though.

Greetings,

Andres Freund

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* Andreas Karlsson (andr...@proxel.se) wrote:
> On 11/27/18 4:11 PM, Stephen Frost wrote:
> >>I agree with your larger point, but in this case the two breakages do not
> >>seem equal. As far as I gather the removal of recovery.conf will in practice
> >>result in a longer downtime when your recovery scripts breaks but not any
> >>data loss. While if we remove the exclusive backup mode then some people's
> >>backup scripts will break and if they do not properly monitor their backups
> >>they risk data loss.
> >
> >Let's please try to remember that this is across a major version upgrade
> >and is removing a broken capability that's already been deprecated for a
> >couple years.
> 
> I know that and you know that, but even our own manual use the exclusive
> mode in some of the examples, e.g: "pg_start_backup('label_goes_here')"[1].

Thankfully, that'll get nicely cleaned up by removing the exclusive
backup mode.

> Your point about that people who do not monitor their backups wont notice
> warnings is fair, but even so I think we should give people more time to
> migrate when even our own documentation isn't always clear about the
> exclusive mode being deprecated.

I don't buy off on this argument- we also have things like this:

https://www.postgresql.org/docs/11/backup-file.html

Where we show a simple tar command as a way to backup the database, but
then we caveat it.  Similairly, we make it clear that users shouldn't be
using the exclusive mode backups:

https://www.postgresql.org/docs/11/continuous-archiving.html#BACKUP-LOWLEVEL-BASE-BACKUP

If users are only reading the system functions description and thinking
that's enough to figure out how to do backups, and didn't read the
chapters of documentation we have about how to perform a backup, then
chances are very good their existing backup systems are broken, and
that's all the more reason to remove the exclusive mode because at least
then it won't look like things are working when they aren't.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[Simon Riggs]

On Tue, 27 Nov 2018 at 14:45, Stephen Frost  wrote:

> Greetings,
>
> * Michael Paquier (mich...@paquier.xyz) wrote:
> > On Tue, Nov 27, 2018 at 02:21:49PM +0100, Peter Eisentraut wrote:
> > > On 27/11/2018 04:46, Andres Freund wrote:
> > >> That was my gut reaction as well, but I think David's argument about
> > >> existing scripts / workflows being broken due the the recovery.conf
> > >> is a good reason to be more aggressive here.
> > >
> > > But backup scripts are not affected by the recovery.conf changes.
> >
> > In any of my own backup scripts (yeah!), I don't have any dependency to
> > that either.  Or perhaps pgBackRest has a dependency in this area?
>
> If you don't consider your recovery scripts and your backup scripts to
> be related then I've really got to wonder how you're regularly testing
> your backups to make sure that they're actually valid.
>
> If you aren't regularly testing your backups then I've got little
> sympathy.
>
> To be clear, pgbackrest doesn't have any dependency here- but it, like
> all of the other 3rd party backup solutions and any restore solution
> that a user has come up with, are going to have to be changed to deal
> with the changes in how recovery works, so this is a good time to make
> these changes.
>

If those tools are updated and the changes all work, that will be good.

That isn't the same thing as an argument to remove things in this release.

I propose waiting until next release.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* David Steele (da...@pgmasters.net) wrote:
> On 11/27/18 10:29 AM, Peter Eisentraut wrote:
> > On 27/11/2018 16:02, Stephen Frost wrote:
> >> They're also the sort of installations which don't have reliable backups
> >> and don't have any clue about the danger they are in due to the current
> >> bug/issue/whatever we have with exclusive backups.
> >>
> >> No, I don't agree that it's sensible to continue to march on as if
> >> nothing is wrong.
> > 
> > It's fair to argue that this facility is broken and needs to be removed.
> >  But that needs to be its own argument.

We made that argument, and had agreement on it, and that's why it's
deprecated today.  The only question here is how long we keep this
deprecated and broken capability around and given that we're completely
breaking everything around recovery, now is as good a time as any since
users should be looking at how they do backup and restore when they
migrate to v12 due to these changes.

> > The argument in this subthread is that since we're already making
> > changes in an adjacent area, removing this feature will have a low impact.
> 
> Fair enough, but I think the argument against exclusive backups stands
> on its own.  Also, I don't see backup and restore as adjacent.  I see
> them as thoroughly intertwined.

Agreed.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[David Steele]

On 11/27/18 8:56 AM, Simon Riggs wrote:
> On Tue, 27 Nov 2018 at 03:13, David Steele  > wrote:
>  
> 
> The deprecated exclusive mode promises to make a difficult problem
> simple but doesn't live up to that promise. That's why it was replaced
> externally in 9.6 and why pg_basebackup has not used exclusive backups
> since it was introduced in 9.2.
> 
> Non-exclusive backups have been available since 9.6 and several
> third-party solutions support this mode, in addition to pg_basebackup.
> 
> The recently introduced recovery changes will break current automated
> solutions so this seems like a good opportunity to make improvements on
> the backup side as well.
> 
> I'll submit a patch for the 2019-01 commitfest.
> 
> 
> -1 for removal, in this release
> 
> It's not there because anyone likes it, it's there because removing it
> is a risk
> 
> Recent changes are the reason to keep it, not a reason to remove.

How so?

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

Re: Remove Deprecated Exclusive Backup Mode

[David Steele]

On 11/27/18 10:29 AM, Peter Eisentraut wrote:
> On 27/11/2018 16:02, Stephen Frost wrote:
>> They're also the sort of installations which don't have reliable backups
>> and don't have any clue about the danger they are in due to the current
>> bug/issue/whatever we have with exclusive backups.
>>
>> No, I don't agree that it's sensible to continue to march on as if
>> nothing is wrong.
> 
> It's fair to argue that this facility is broken and needs to be removed.
>  But that needs to be its own argument.

I believe I made this argument in the OP.

> The argument in this subthread is that since we're already making
> changes in an adjacent area, removing this feature will have a low impact.

Fair enough, but I think the argument against exclusive backups stands
on its own.  Also, I don't see backup and restore as adjacent.  I see
them as thoroughly intertwined.

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

Re: shared-memory based stats collector

[Tomas Vondra]

On 11/27/18 9:59 AM, Kyotaro HORIGUCHI wrote:
>>
>> ...>>

For the main workload there's pretty much no difference, but for selects
from the stats catalogs there's ~20% drop in throughput. In absolute
numbers this means drop from ~670tps to ~550tps. I haven't investigated
this, but I suppose this is due to dshash seqscan being more expensive
than reading the data from file.


Thanks for finding that. The three seqscan loops in
pgstat_vacuum_stat cannot take such a long time, I think. I'll
investigate it.



OK. I'm not sure this is related to pgstat_vacuum_stat - the slowdown 
happens while querying the catalogs, so why would that trigger vacuum of 
the stats? I may be missing something, of course.


FWIW, the "query statistics" test simply does this:

  SELECT * FROM pg_stat_all_tables;
  SELECT * FROM pg_stat_all_indexes;
  SELECT * FROM pg_stat_user_indexes;
  SELECT * FROM pg_stat_user_tables;
  SELECT * FROM pg_stat_sys_tables;
  SELECT * FROM pg_stat_sys_indexes;

and the slowdown happened even it was running on it's own (nothing else 
running on the instance). Which mostly rules out concurrency issues with 
the hash table locking etc.



I don't think any of this is an issue in practice, though. The important
thing is that there's no measurable impact on the regular workload.

Now, a couple of comments regarding individual parts of the patch.


0001-0003
-

I do think 0001 - 0003 are ready, with some minor cosmetic issues:

1) I'd rephrase the last part of dshash_seq_init comment more like this:

* If consistent is set for dshash_seq_init, the all hash table
* partitions are locked in the requested mode (as determined by the
* exclusive flag), and the locks are held until the end of the scan.
* Otherwise the partition locks are acquired and released as needed
* during the scan (up to two partitions may be locked at the same time).


Replaced with this.


Maybe it should briefly explain what the consistency guarantees are (and
aren't), but considering we're not materially changing the existing
behavior probably  is not really necessary.


Mmm. actually sequential scan is a new thing altogether, but..



Sure, there are new pieces. But does it significantly change consistency 
guarantees when reading the stats? I don't think so - there was no 
strict consistency guaranteed before (due to data interleaved with 
inquiries, UDP throwing away packets under load, etc.). Based on the 
discussion in this thread that seems to be the consensus.



2) I think the dshash_find_extended() signature should be more like
dshash_find(), i.e. just appending parameters instead of moving them
around unnecessarily. Perhaps we should add


Sure. It seems to have done by my off-lined finger;p Fixed.



;-)



0004 (+0005 and 0007)
-

This seems fine, but I have my doubts about two changes - removing of
stats_temp_directory and the IDLE_STATS_UPDATE_TIMEOUT thingy.

There's a couple of issues with the stats_temp_directory. Firstly, I
don't understand why it's spread over multiple parts of the patch. The
GUC is removed in 0004, the underlying variable is removed in 0005 and
then the docs are updated in 0007. If we really want to do this, it
should happen in a single patch.


Sure.


But the main question is - do we really want to do that? I understand
this directory was meant for the stats data we're moving to shared
memory, so removing it seems natural. But clearly it's used by
pg_stat_statements - 0005 fixes that, of course, but I wonder if there
are other extensions using it to store files?
It's not just about how intensive I/O to those files is, but this also
means the files will now be included in backups / pg_rewind, and maybe
that's not really desirable?

Maybe it's fine but I'm not quite convinced about it ...


It was also in my mind. Anyway sorry for the strange separation.

I was confused about pgstat_stat_directory (the names are
actually very confusing..). Addition to that pg_stat_statements
does *not* use the variable stats_temp_directory, but using
PG_STAT_TMP_DIR. pgstat_stat_directory was used only by
basebackup.c.

The GUC base variable pgstat_temp_directory is not extern'ed so
we can just remove it along with the GUC
definition. pgstat_stat_directory (it actually stores *temporary*
stats directory) was extern'ed in pgstat.h and PG_STAT_TMP_DIR is
defined in pgstat.h. They are not removed in the new version.
Finally 0005 no longer breaks any other bins, contribs and
external extensions.



Great. I'll take a look.


I'm not sure I understand what IDLE_STATS_UPDATE_TIMEOUT does. You've
described it as

This adds a new timeout IDLE_STATS_UPDATE_TIMEOUT. This works
similarly to IDLE_IN_TRANSACTIION_SESSION_TIMEOUT. It fires in
at most PGSTAT_STAT_MIN_INTERVAL(500)ms to clean up pending
statistics update.

but I'm not sure what pending updates do you mean? Aren't we updating
the stats at the end of each transaction? At least that's what we've
been doing before, so may

Re: Remove Deprecated Exclusive Backup Mode

[David Steele]

On 11/27/18 9:54 AM, Peter Eisentraut wrote:
> On 27/11/2018 15:45, Stephen Frost wrote:
 But backup scripts are not affected by the recovery.conf changes.
>>> In any of my own backup scripts (yeah!), I don't have any dependency to
>>> that either.  Or perhaps pgBackRest has a dependency in this area?
>> If you don't consider your recovery scripts and your backup scripts to
>> be related then I've really got to wonder how you're regularly testing
>> your backups to make sure that they're actually valid.
> 
> The sort of installations that continue to use the exclusive backup mode
> probably have the following tooling: a 20-line shell script to make the
> backup and either a 10-line shell script or a similarly sized README or
> wiki page to do the recovery.  Changing the latter for the recovery.conf
> changes is probably a 3-line change.  

Really?  We have gone from a file that can be safely overwritten
(recovery.conf) to a file that might need to be parsed to figure out if
the settings already exists (postgresql.auto.conf).  Of course, you can
just continue to append to the file but that seems pretty grotty to me.

This will also require changes to all HA solutions or backup solutions
that deal with recovery.  I think you underestimate how big a change
this is.  I've been thinking about how to write code for it and it is
significantly more complex -- also more flexible so I'm happy about that.

> Changing the former for the
> removal of exclusive backups would require major changes.  (Try writing
> a shell script that keeps a psql session open while it takes the backup
> from the file system.  It's possible, but it requires significantly more
> logic.)

We provide pg_basebackup with core and it is a solid tool for doing
backups.  Do we really want to encourage the use of bash scripts to do
what we already have a tool for?  If users want to do something more
complex than pg_basebackup then bash is certainly not the tool for that.

Regards,
-- 
-David
da...@pgmasters.net

Re: Continue work on changes to recovery.conf API

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 27/11/2018 13:21, David Steele wrote:
> > I would prefer a specific file that will be auto-included into
> > postgresql.conf when present but be ignored when not present.  Some
> > settings are generally ephemeral (recovery_target_time) and it would be
> > nice for them to go away.  When recovery is complete the file would be
> > renamed to .done just as recovery.conf is now.
> 
> That might be a useful facility, but it wouldn't really address the
> pg_basebackup -R issue, because that creates settings that you don't
> want going away in this manner.  You'd then need two separate such
> files, one for targeted recovery that goes away when recovery ends, and
> one that is automatically included that pg_basebackup can overwrite at will.

I've been thinking about this also and I agree that there's some
challenges when it comes to having another file- what happens if someone
does an ALTER SYSTEM on primary_conninfo while it's in the
'recovery.auto.conf' (or whatever) file?  Does that go into
postgresql.auto.conf, or somewhere else?  What if they do a RESET?

Then there's the other side of things- do we really want things like
recovery_target_time being kept around in postgresql.auto.conf after a
promotoion?  Do we want to keep appending primary_conninfo into
postgresql.auto.conf?  I haven't looked but I'm also concerned that
something like ALTER SYSTEM RESET isn't really prepared to find
duplicates in postgresql.auto.conf...

Maybe thinking through what we want to have happen in each scenario
would be good.  If you perform a pg_basebackup -R and there's already
something set in postgresql.auto.conf for primary conninfo- what should
happen?  If we reach the end of recovery and promote while
postgresql.auto.conf has recovery_target_time set, what should happen?
If third-party tools want to do what pg_basebackup -R does and modify
things in postgresql.auto.conf, how should they do that?

Here's my thoughts on answers to the above:

- pg_basebackup -R should INSERT .. ON CONFLICT UPDATE the settings that
  it wants to set in postgresql.auto.conf

- When we reach the end of recovery and promote, we should DELETE from
  the postgresql.auto.conf the recovery target settings.

- External tools should either be told that they can modify
  postgresql.auto.conf and given guideance on how to do so, or we should
  provide a tool which allows them to do so (or maybe both).

As we already have a section for recovery target settings that clearly
has them as independent, hopefully this will make sense to users.  Open
to other ideas too, of course, but I don't think we can continue to just
append things to the end of postgresql.auto.conf when pg_basebackup is
run with -R.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[Peter Eisentraut]

On 27/11/2018 16:02, Stephen Frost wrote:
> They're also the sort of installations which don't have reliable backups
> and don't have any clue about the danger they are in due to the current
> bug/issue/whatever we have with exclusive backups.
> 
> No, I don't agree that it's sensible to continue to march on as if
> nothing is wrong.

It's fair to argue that this facility is broken and needs to be removed.
 But that needs to be its own argument.

The argument in this subthread is that since we're already making
changes in an adjacent area, removing this feature will have a low impact.

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

Re: pgsql: Add TAP tests for pg_verify_checksums

[Michael Banck]

Hi,

Am Dienstag, den 27.11.2018, 22:52 +0900 schrieb Michael Paquier:
> On Tue, Nov 27, 2018 at 02:09:05PM +0100, Michael Banck wrote:
> > I had a quick look at fixing this but did not manage to immediately come
> > up with a solution, so posting here for now.
> 
> If you look at another thread, the patch posted on the top would
> actually solve this issue:
> https://www.postgresql.org/message-id/20181021134206.ga14...@paquier.xyz

Oh, I kinda followed that thread a bit, but I think that patch fixes
things more by matter of moving code around, at least I haven't noticed
that tablespaces were explicitly claimed to be fixed in that thread.

> Your problem could also be solved with the minimalistic patch attached,
> so fixing on the way the problems with temporary files present in PGDATA
> something like the attached could be used...  

Thanks!

> Based on the stale status
> of the other thread I am unsure what should be done though.

As pg_verify_checksums appears to be broken with respect to tablespaces
in REL_11_STABLE (so I think 11.1, but not 11.0) as well, I think this
merits a short-term minimal invasive fix (i.e. the patch you posted,
just that there's no TAP testsuite for pg_verify_checksums in v11
unfortunately) on its own, regardless of the wider changes proposed in
the other thread.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Re: Remove Deprecated Exclusive Backup Mode

[Andreas Karlsson]

On 11/27/18 4:11 PM, Stephen Frost wrote:

I agree with your larger point, but in this case the two breakages do not
seem equal. As far as I gather the removal of recovery.conf will in practice
result in a longer downtime when your recovery scripts breaks but not any
data loss. While if we remove the exclusive backup mode then some people's
backup scripts will break and if they do not properly monitor their backups
they risk data loss.


Let's please try to remember that this is across a major version upgrade
and is removing a broken capability that's already been deprecated for a
couple years.


I know that and you know that, but even our own manual use the exclusive 
mode in some of the examples, e.g: "pg_start_backup('label_goes_here')"[1].


Your point about that people who do not monitor their backups wont 
notice warnings is fair, but even so I think we should give people more 
time to migrate when even our own documentation isn't always clear about 
the exclusive mode being deprecated.


1. 
https://www.postgresql.org/docs/11/functions-admin.html#FUNCTIONS-ADMIN-BACKUP


Andreas

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* Andreas Karlsson (andr...@proxel.se) wrote:
> On 11/27/18 3:46 PM, Stephen Frost wrote:
> >I'm concerned, seriously, that people don't have anywhere near the
> >concern about the recovery side of things as they do about the backup
> >side of things and that's really concerning.
> 
> I agree with your larger point, but in this case the two breakages do not
> seem equal. As far as I gather the removal of recovery.conf will in practice
> result in a longer downtime when your recovery scripts breaks but not any
> data loss. While if we remove the exclusive backup mode then some people's
> backup scripts will break and if they do not properly monitor their backups
> they risk data loss.

Let's please try to remember that this is across a major version upgrade
and is removing a broken capability that's already been deprecated for a
couple years.

If they aren't monitoring their backup scripts today, and aren't
regularly performing restores of those backups, they're already risking
data loss.

> I think we should use more caution when data loss is at stake rather than
> "just" downtime. So personally I am in favor of updating the manual with
> warnings (right now it does not even say if exclusive or non-exclusive is
> the default) and adding a deprecation warning when people use the exclusive
> mode.

Waiting isn't going to change any of these factors, nor will throwing
warnings about the exclusive mode if people aren't monitoring their
scripts.

If we really want to keep the exclusive backup mode around, then, as
Magnus said on a nearby thread, it needs to be fixed.  If it's fixed and
just works and everyone's scripts work, then great, then we can
un-deprecate it and move on.

If we aren't going to fix it then let's remove it.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[Andreas Karlsson]

On 11/27/18 3:46 PM, Stephen Frost wrote:

I'm concerned, seriously, that people don't have anywhere near the
concern about the recovery side of things as they do about the backup
side of things and that's really concerning.


I agree with your larger point, but in this case the two breakages do 
not seem equal. As far as I gather the removal of recovery.conf will in 
practice result in a longer downtime when your recovery scripts breaks 
but not any data loss. While if we remove the exclusive backup mode then 
some people's backup scripts will break and if they do not properly 
monitor their backups they risk data loss.


I think we should use more caution when data loss is at stake rather 
than "just" downtime. So personally I am in favor of updating the manual 
with warnings (right now it does not even say if exclusive or 
non-exclusive is the default) and adding a deprecation warning when 
people use the exclusive mode.


Best regards,
Andreas

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 27/11/2018 15:45, Stephen Frost wrote:
> >>> But backup scripts are not affected by the recovery.conf changes.
> >> In any of my own backup scripts (yeah!), I don't have any dependency to
> >> that either.  Or perhaps pgBackRest has a dependency in this area?
> > If you don't consider your recovery scripts and your backup scripts to
> > be related then I've really got to wonder how you're regularly testing
> > your backups to make sure that they're actually valid.
> 
> The sort of installations that continue to use the exclusive backup mode
> probably have the following tooling: a 20-line shell script to make the
> backup and either a 10-line shell script or a similarly sized README or
> wiki page to do the recovery.  Changing the latter for the recovery.conf
> changes is probably a 3-line change.  Changing the former for the
> removal of exclusive backups would require major changes.  (Try writing
> a shell script that keeps a psql session open while it takes the backup
> from the file system.  It's possible, but it requires significantly more
> logic.)

They're also the sort of installations which don't have reliable backups
and don't have any clue about the danger they are in due to the current
bug/issue/whatever we have with exclusive backups.

No, I don't agree that it's sensible to continue to march on as if
nothing is wrong.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[Peter Eisentraut]

On 27/11/2018 15:45, Stephen Frost wrote:
>>> But backup scripts are not affected by the recovery.conf changes.
>> In any of my own backup scripts (yeah!), I don't have any dependency to
>> that either.  Or perhaps pgBackRest has a dependency in this area?
> If you don't consider your recovery scripts and your backup scripts to
> be related then I've really got to wonder how you're regularly testing
> your backups to make sure that they're actually valid.

The sort of installations that continue to use the exclusive backup mode
probably have the following tooling: a 20-line shell script to make the
backup and either a 10-line shell script or a similarly sized README or
wiki page to do the recovery.  Changing the latter for the recovery.conf
changes is probably a 3-line change.  Changing the former for the
removal of exclusive backups would require major changes.  (Try writing
a shell script that keeps a psql session open while it takes the backup
from the file system.  It's possible, but it requires significantly more
logic.)

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

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> There must be hundreds or thousands of backup scripts written by
> individual users that still use exclusive mode floating around out
> there.  Forcing all of those to be updated or scrapped will annoy
> users to no benefit.

How about automated recovery scripts?

We've decided it's fine to break all of those which exist out there.

I'm concerned, seriously, that people don't have anywhere near the
concern about the recovery side of things as they do about the backup
side of things and that's really concerning.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Remove Deprecated Exclusive Backup Mode

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Nov 27, 2018 at 02:21:49PM +0100, Peter Eisentraut wrote:
> > On 27/11/2018 04:46, Andres Freund wrote:
> >> That was my gut reaction as well, but I think David's argument about
> >> existing scripts / workflows being broken due the the recovery.conf
> >> is a good reason to be more aggressive here.
> > 
> > But backup scripts are not affected by the recovery.conf changes.
> 
> In any of my own backup scripts (yeah!), I don't have any dependency to
> that either.  Or perhaps pgBackRest has a dependency in this area?

If you don't consider your recovery scripts and your backup scripts to
be related then I've really got to wonder how you're regularly testing
your backups to make sure that they're actually valid.

If you aren't regularly testing your backups then I've got little
sympathy.

To be clear, pgbackrest doesn't have any dependency here- but it, like
all of the other 3rd party backup solutions and any restore solution
that a user has come up with, are going to have to be changed to deal
with the changes in how recovery works, so this is a good time to make
these changes.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: pgsql: Integrate recovery.conf into postgresql.conf

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 27/11/2018 13:29, Peter Eisentraut wrote:
> > On 27/11/2018 10:10, Sergei Kornilov wrote:
>   - recovery_target = immediate was replaced with 
>  recovery_target_immediate bool GUC
> >>>
> >>> Why?
> >> Due this comment: 
> >> https://www.postgresql.org/message-id/20181126172118.GY3415%40tamriel.snowman.net
> >>> I've not been following this very closely, but seems like
> >>> recovery_target_string is a bad idea.. Why not just make that
> >>> 'recovery_target_immediate' and make it a boolean? Having a GUC that's
> >>> only got one valid value seems really odd.
> > 
> > It is a bit odd, but that's the way it's been, and I don't see a reason
> > to change it as part of this fix.  We are attempting to fix the way the
> > GUC parameters are parsed, not change the name and meaning of the
> > parameters themselves.

I disagree quite a bit- we're whacking around how recovery works and
this has been a wart since it went in because the contemplated later
changes to have more than one value be accepted there never
materialized, so we might as well change it while we're changing
everything else and clean it up.

If we really want to change it later we can do that, but we've evidently
decided to go the opposite direction and have multiple GUCs instead, so
let's be consistent.

> The attached seems to be the simplest way to fix this.  (Needs
> documentation updates, test updates, error message refinement.)

Sure, this approach seems fine to me and is perhaps a bit cleaner.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: SSL tests failing with "ee key too small" error on Debian SID

[Tom Lane]

Peter Eisentraut  writes:
> On 01/10/2018 14:18, Kyotaro HORIGUCHI wrote:
>> The attached second patch just changes key size to 2048 bits and
>> "ee key too small" are eliminated in 001_ssltests_master, but
>> instead I got "ca md too weak" error. This is eliminated by using
>> sha256 instead of sha1 in cas.config. (third attached)

> I have applied these configuration changes and created a new set of test
> files with them.

Buildfarm critters aren't going to be happy unless you back-patch that.

regards, tom lane

Re: Continue work on changes to recovery.conf API

[Peter Eisentraut]

On 27/11/2018 13:21, David Steele wrote:
> I would prefer a specific file that will be auto-included into
> postgresql.conf when present but be ignored when not present.  Some
> settings are generally ephemeral (recovery_target_time) and it would be
> nice for them to go away.  When recovery is complete the file would be
> renamed to .done just as recovery.conf is now.

That might be a useful facility, but it wouldn't really address the
pg_basebackup -R issue, because that creates settings that you don't
want going away in this manner.  You'd then need two separate such
files, one for targeted recovery that goes away when recovery ends, and
one that is automatically included that pg_basebackup can overwrite at will.

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

Re: SSL tests failing with "ee key too small" error on Debian SID

[Peter Eisentraut]

On 01/10/2018 14:18, Kyotaro HORIGUCHI wrote:
> By the way I got (with both 1.0.2k and 1.1.1) a "tlsv1 alert
> unknown ca" error from 002_scram.pl. It is fixed for me by the
> forth attached, but I'm not sure why we haven't have such a
> complain. (It happens only for me?)

I haven't seen it.  Do the tests print that out or does it appear in the
logs?  Which test complains?

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

Re: SSL tests failing with "ee key too small" error on Debian SID

[Peter Eisentraut]

On 01/10/2018 14:18, Kyotaro HORIGUCHI wrote:
> The attached second patch just changes key size to 2048 bits and
> "ee key too small" are eliminated in 001_ssltests_master, but
> instead I got "ca md too weak" error. This is eliminated by using
> sha256 instead of sha1 in cas.config. (third attached)

I have applied these configuration changes and created a new set of test
files with them.

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

Re: pgsql: Integrate recovery.conf into postgresql.conf

[Sergei Kornilov]

Hi

> The attached seems to be the simplest way to fix this. (Needs
> documentation updates, test updates, error message refinement.)

Thank you!
I add documentation change, tests and rephrased error message.

regards, Sergeidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index db1a2d4..7cbea6e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3221,7 +3221,7 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
   recovery_target_lsn, recovery_target_name,
   recovery_target_time, or recovery_target_xid
   can be used; if more than one of these is specified in the configuration
-  file, the last entry will be used.
+  file, a error will be raised.
   These parameters can only be set at server start.
  
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6497393..290a157 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11065,14 +11065,11 @@ check_recovery_target(char **newval, void **extra, GucSource source)
 static void
 assign_recovery_target(const char *newval, void *extra)
 {
+	if (recoveryTarget)
+		elog(ERROR, "can not specify multiple recovery targets");
+
 	if (newval && strcmp(newval, "") != 0)
 		recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
-	else
-		/*
-		 * Reset recoveryTarget to RECOVERY_TARGET_UNSET to proper handle user
-		 * setting multiple recovery_target with blank value on last.
-		 */
-		recoveryTarget = RECOVERY_TARGET_UNSET;
 }
 
 static bool
@@ -11098,13 +11095,14 @@ check_recovery_target_xid(char **newval, void **extra, GucSource source)
 static void
 assign_recovery_target_xid(const char *newval, void *extra)
 {
+	if (recoveryTarget)
+		elog(ERROR, "can not specify multiple recovery targets");
+
 	if (newval && strcmp(newval, "") != 0)
 	{
 		recoveryTarget = RECOVERY_TARGET_XID;
 		recoveryTargetXid = *((TransactionId *) extra);
 	}
-	else
-		recoveryTarget = RECOVERY_TARGET_UNSET;
 }
 
 static bool
@@ -11161,13 +11159,14 @@ check_recovery_target_time(char **newval, void **extra, GucSource source)
 static void
 assign_recovery_target_time(const char *newval, void *extra)
 {
+	if (recoveryTarget)
+		elog(ERROR, "can not specify multiple recovery targets");
+
 	if (newval && strcmp(newval, "") != 0)
 	{
 		recoveryTarget = RECOVERY_TARGET_TIME;
 		recoveryTargetTime = *((TimestampTz *) extra);
 	}
-	else
-		recoveryTarget = RECOVERY_TARGET_UNSET;
 }
 
 static bool
@@ -11186,13 +11185,14 @@ check_recovery_target_name(char **newval, void **extra, GucSource source)
 static void
 assign_recovery_target_name(const char *newval, void *extra)
 {
+	if (recoveryTarget)
+		elog(ERROR, "can not specify multiple recovery targets");
+
 	if (newval && strcmp(newval, "") != 0)
 	{
 		recoveryTarget = RECOVERY_TARGET_NAME;
 		recoveryTargetName = (char *) newval;
 	}
-	else
-		recoveryTarget = RECOVERY_TARGET_UNSET;
 }
 
 static bool
@@ -11240,13 +11240,14 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
 static void
 assign_recovery_target_lsn(const char *newval, void *extra)
 {
+	if (recoveryTarget)
+		elog(ERROR, "can not specify multiple recovery targets");
+
 	if (newval && strcmp(newval, "") != 0)
 	{
 		recoveryTarget = RECOVERY_TARGET_LSN;
 		recoveryTargetLSN = *((XLogRecPtr *) extra);
 	}
-	else
-		recoveryTarget = RECOVERY_TARGET_UNSET;
 }
 
 static bool
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index f3a7ba4..1fd4aec 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -79,7 +79,7 @@ extern HotStandbyState standbyState;
  */
 typedef enum
 {
-	RECOVERY_TARGET_UNSET,
+	RECOVERY_TARGET_UNSET = 0,
 	RECOVERY_TARGET_XID,
 	RECOVERY_TARGET_TIME,
 	RECOVERY_TARGET_NAME,
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index f6f2e8b..cba367c 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 9;
+use Test::More tests => 10;
 
 # Create and test a standby from given backup, with a certain recovery target.
 # Choose $until_lsn later than the transaction commit that causes the row
@@ -115,31 +115,44 @@ test_recovery_standby('name', 'standby_4', $node_master, \@recovery_params,
 test_recovery_standby('LSN', 'standby_5', $node_master, \@recovery_params,
 	"5000", $lsn5);
 
-# Multiple targets
-# Last entry has priority (note that an array respects the order of items
-# not hashes).
+# multiple recovery targets are disallowed
+sub test_recovery_multiple_targets
+{
+	my $test_name   = shift;
+	my $node_name   = shift;
+	my $node_master = shift;
+	my $recovery_params = shift;
+
+	my $node_standby = get_new_node($node_name);
+	$node_standby->init_from_backup($node_master, 'my_backup',
+		has_restoring => 1);
+
+	foreach my $param_item (@$recovery_params)
+	{
+		$node_

Re: Remove Deprecated Exclusive Backup Mode

[Simon Riggs]

On Tue, 27 Nov 2018 at 03:13, David Steele  wrote:


> The deprecated exclusive mode promises to make a difficult problem
> simple but doesn't live up to that promise. That's why it was replaced
> externally in 9.6 and why pg_basebackup has not used exclusive backups
> since it was introduced in 9.2.
>
> Non-exclusive backups have been available since 9.6 and several
> third-party solutions support this mode, in addition to pg_basebackup.
>
> The recently introduced recovery changes will break current automated
> solutions so this seems like a good opportunity to make improvements on
> the backup side as well.
>
> I'll submit a patch for the 2019-01 commitfest.
>

-1 for removal, in this release

It's not there because anyone likes it, it's there because removing it is a
risk

Recent changes are the reason to keep it, not a reason to remove.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove Deprecated Exclusive Backup Mode

[Michael Paquier]

On Tue, Nov 27, 2018 at 02:21:49PM +0100, Peter Eisentraut wrote:
> On 27/11/2018 04:46, Andres Freund wrote:
>> That was my gut reaction as well, but I think David's argument about
>> existing scripts / workflows being broken due the the recovery.conf
>> is a good reason to be more aggressive here.
> 
> But backup scripts are not affected by the recovery.conf changes.

In any of my own backup scripts (yeah!), I don't have any dependency to
that either.  Or perhaps pgBackRest has a dependency in this area?
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Add TAP tests for pg_verify_checksums

[Michael Paquier]

On Tue, Nov 27, 2018 at 02:09:05PM +0100, Michael Banck wrote:
> I had a quick look at fixing this but did not manage to immediately come
> up with a solution, so posting here for now.

If you look at another thread, the patch posted on the top would
actually solve this issue:
https://www.postgresql.org/message-id/20181021134206.ga14...@paquier.xyz

Your problem could also be solved with the minimalistic patch attached,
so fixing on the way the problems with temporary files present in PGDATA
something like the attached could be used...  Based on the stale status
of the other thread I am unsure what should be done though.
--
Michael
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index f0e09bea20..148aa511f6 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -21,6 +21,7 @@
 #include "storage/bufpage.h"
 #include "storage/checksum.h"
 #include "storage/checksum_impl.h"
+#include "storage/fd.h"
 
 
 static int64 files = 0;
@@ -189,9 +190,22 @@ scan_directory(const char *basedir, const char *subdir)
 		char		fn[MAXPGPATH];
 		struct stat st;
 
-		if (!isRelFileName(de->d_name))
+		if (strcmp(de->d_name, ".") == 0 ||
+			strcmp(de->d_name, "..") == 0)
 			continue;
 
+		/* Skip temporary files */
+		if (strncmp(de->d_name,
+	PG_TEMP_FILE_PREFIX,
+	strlen(PG_TEMP_FILE_PREFIX)) == 0)
+			continue;
+
+		/* Skip temporary folders */
+		if (strncmp(de->d_name,
+	PG_TEMP_FILES_DIR,
+	strlen(PG_TEMP_FILES_DIR)) == 0)
+			return;
+
 		snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
 		if (lstat(fn, &st) < 0)
 		{
@@ -206,6 +220,13 @@ scan_directory(const char *basedir, const char *subdir)
 	   *segmentpath;
 			BlockNumber segmentno = 0;
 
+			/*
+			 * Only normal relation files can be analyzed.  Note that this
+			 * skips temporary relations.
+			 */
+			if (!isRelFileName(de->d_name))
+continue;
+
 			/*
 			 * Cut off at the segment boundary (".") to get the segment number
 			 * in order to mix it into the checksum. Then also cut off at the
diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl
index 0e1725d9f2..fd64de050e 100644
--- a/src/bin/pg_verify_checksums/t/002_actions.pl
+++ b/src/bin/pg_verify_checksums/t/002_actions.pl
@@ -5,7 +5,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 36;
+use Test::More tests => 42;
 
 # Initialize node with checksums enabled.
 my $node = get_new_node('node_checksum');
@@ -54,31 +54,7 @@ command_fails(['pg_verify_checksums',  '-D', $pgdata],
 			  "fails with online cluster");
 
 # Create table to corrupt and get its relfilenode
-$node->safe_psql('postgres',
-	"SELECT a INTO corrupt1 FROM generate_series(1,1) AS a;
-	ALTER TABLE corrupt1 SET (autovacuum_enabled=false);");
-
-my $file_corrupted = $node->safe_psql('postgres',
-	"SELECT pg_relation_filepath('corrupt1')");
-my $relfilenode_corrupted =  $node->safe_psql('postgres',
-	"SELECT relfilenode FROM pg_class WHERE relname = 'corrupt1';");
-
-# Set page header and block size
-my $pageheader_size = 24;
-my $block_size = $node->safe_psql('postgres', 'SHOW block_size;');
-$node->stop;
-
-# Checksums are correct for single relfilenode as the table is not
-# corrupted yet.
-command_ok(['pg_verify_checksums',  '-D', $pgdata,
-	'-r', $relfilenode_corrupted],
-	"succeeds for single relfilenode with offline cluster");
-
-# Time to create some corruption
-open my $file, '+<', "$pgdata/$file_corrupted";
-seek($file, $pageheader_size, 0);
-syswrite($file, '\0\0\0\0\0\0\0\0\0');
-close $file;
+my $relfilenode_corrupted = create_corruption($node, 'corrupt1', 'pg_default');
 
 # Global checksum checks fail
 $node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata],
@@ -95,6 +71,72 @@ $node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata, '-r',
 		  [qr/checksum verification failed/],
 		  'fails for corrupted data on single relfilenode');
 
+# Drop corrupt table again and make sure there is no more corruption
+$node->start;
+$node->safe_psql('postgres', 'DROP TABLE corrupt1;');
+$node->stop;
+$node->command_ok(['pg_verify_checksums', '-D', $pgdata],
+'succeeds again: '.$node->data_dir);
+
+# Create table to corrupt in a non-default tablespace and get its relfilenode
+my $tablespace_dir = $node->data_dir."/../ts_corrupt_dir";
+mkdir ($tablespace_dir);
+$node->start;
+$node->safe_psql('postgres', "CREATE TABLESPACE ts_corrupt LOCATION '".$tablespace_dir."';");
+$relfilenode_corrupted = create_corruption($node, 'corrupt2', 'ts_corrupt');
+$node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata],
+		  1,
+		  [qr/Bad checksums:.*1/],
+		  [qr/checksum verification failed/],
+		  'fails with corrupted data in non-default tablespace');
+
+# Drop corrupt table again and make sure there is no more corruption
+$node->start;
+$nod

Re: SSL tests failing with "ee key too small" error on Debian SID

[Peter Eisentraut]

On 26/11/2018 01:35, Michael Paquier wrote:
> When going up to 2k, it takes longer to generate the keys than to run
> the tests, so keeping them in the tree looks like a pretty good gain to
> me.

Another concern might be that repeatedly generating certificates might
drain entropy unnecessarily.

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

Re: Remove Deprecated Exclusive Backup Mode

[Peter Eisentraut]

On 27/11/2018 04:46, Andres Freund wrote:
> That was my gut reaction as well, but I think David's argument about
> existing scripts / workflows being broken due the the recovery.conf
> is a good reason to be more aggressive here.

But backup scripts are not affected by the recovery.conf changes.

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

Re: Python versions (was Re: RHEL 8.0 build)

[Peter Eisentraut]

On 25/11/2018 23:14, Tom Lane wrote:
> Andres Freund  writes:
>> On 2018-11-24 15:49:25 -0500, Tom Lane wrote:
>>> There's been some preliminary discussion about starting to default to
>>> python3, but given this project's inherent conservatism, I don't expect
>>> that to happen for some years yet.  In any case, whenever we do pull
>>> that trigger we'd surely do so only in HEAD not released branches, so
>>> buildfarm owners will need to deal with the case for years more.
> 
>> Why don't we probe for python2 in addition to python by default? That
>> ought to make RHEL 8 work, without making the switch just yet.
> 
> I'm unexcited about that because that *would* be expressing a version
> preference --- and one that's on the wrong side of history.

I think it would be appropriate to probe in the order

python python3 python2

This would satisfy most scenarios that are valid under PEP 394.

> Also, I noticed on a fresh FreeBSD 12.0 installation that what
> I've got is
> 
> $ ls /usr/bin/pyth*
> ls: /usr/bin/pyth*: No such file or directory
> $ ls /usr/local/bin/pyth*
> /usr/local/bin/python2.7
> /usr/local/bin/python2.7-config
> /usr/local/bin/python3.6
> /usr/local/bin/python3.6-config
> /usr/local/bin/python3.6m
> /usr/local/bin/python3.6m-config
> 
> So there are modern platforms on which "python2" isn't going to make
> it work automatically either.

I don't think this is a setup we need to support.  You are probably
suppose to install a meta package that will provide a "python" or
"python3" binary.

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

Re: [RFC] Removing "magic" oids

[Andrew Dunstan]

On 11/26/18 5:50 PM, Andres Freund wrote:



With some adjustments to the tests to remove problematic cases (e.g.
postgres_fdw's ft_pg_type) the tests pass. The exception is
HEAD->HEAD. The change is that the LOs are not dumped in the same
order pre and post upgrade. I can change the tests to allow for a
greater fuzz factor - generally when the source and target are the
same we don't allow any fuzz.  Or if we care we could do a better job
of dumping LOs in a consistent order.

So you'd want to dump large objects in oid order or such? Probably
comparatively not a huge overhead, but also not nothing? We don't really
force ordering in other places in pg_dump afaik.


Well, all other data is dumped in a consistent order, and the tests rely on
this. If we don't care about that for LOs I can accommodate it. I don't have
a terribly strong opinion about the desirability of making LOs keep the same
behaviour.

I don't think it's true that other comparable metadata is dumped in a
really consistent order. What changes is the order of data in a system
catalog (pg_largeobject_metadata), and we don't enforce that the order
stays the same in e.g. pg_class either.  I guess we could add a
not-actually-needed sort to getBlobs(), with a comment saying that we
only need that for better comparability and note that it's less needed
for other types of objects due to the dependency ordering that pg_dump
does for most object types.




As of now, I have simnply got around the issue in the buildfarm module 
by allowing up to 50 lines of fuzz in the pre-upgrade and post-upgrade 
diffs when the source and destination branch are the same.



cheers


andrew


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