RE: libpq debug log

[Iwata, Aya]

Hi,

I created a new version patch. Please find attached my patch.

Changes:
Since v2 patch
I forgot to write the detail of v2 patch changes. So I write these.
- Fixed the " Start receiving message from backend:" message location. Because 
I found that message located at outside of "retry4". 
- Changed location where output "start :" / "end :" messages and timestamp.　The 
reason of the change is that v1 patch did not correspond to Asynchronous 
Command Processing.
- Added documentation
- Added documentation how to check mistake of logdir and/or logsize. (Based on 
review comment of Jim san's)
Since v3 patch
- Fixed my mistake on fe-connect.c. Time and message were output at the place 
where does not output in originally PQtrace(). These messages are needed only 
new trace log. So I fixed it.
- Fixed two points so that new trace log overlaps all places in PQtrace(). 
(Based on review comment of Jacob san's)

TODO:
I will add the feature called "logging level" on next version patch. I will 
attach it as soon as possible.

I'm marking it as "Needs review".

Regards,
Aya Iwata


v3-0001-libpq-trace-log.patch
Description: v3-0001-libpq-trace-log.patch

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

[rajan]

Hi,

Please suggest me on the following,

1. Is it better to configure autovacuum threshold at table level?
2. Is there any discussions in this forum which I can refer for
understanding vacuum/autovacuum?

Thanks in advance.
Rajan.



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

Re: Inadequate executor locking of indexes

[Amit Langote]

On 2018/11/27 6:19, David Rowley wrote:
> On Mon, 26 Nov 2018 at 18:57, Amit Langote
>  wrote:
>> On 2018/11/26 14:25, David Rowley wrote:
>>> I'm making efforts to delay per-partition work even further in the
>>> executor, for example locking of the per-partition result relations
>>> until after run-time pruning would be a significant win for
>>> partitioned tables with many partitions when generic plans are in use.
>>> Moving things back to InitPlan() flies in the face of that work.
>>>
>>> [1] https://commitfest.postgresql.org/20/1778/
>>
>> That's an interesting point.  Although, considering the concerns that Tom
>> raised about the same index relation being locked such that lock-strength
>> upgrade occurs (his example containing two CTEs), we'll have to find a way
>> to do the ModifyTable run-time pruning such that result relations and
>> their indexes (possibly under multiple ModifyTable nodes) are locked with
>> RowExclusiveLock before they're locked with AccessShareLock lock as scan
>> relations.  For example, we might be able to find a way to do the
>> ModifyTable run-time pruning in InitPlan before initializing plan trees.
> 
> I thought my idea of the processing the rangetable at the end of
> planning to determine the strongest lock per relation would have
> solved that.

Yeah, would be nice to make that work.

Thanks,
Amit

Re: Pluggable Storage - Andres's take

[Amit Langote]

Hi,

On 2018/11/02 9:17, Haribabu Kommi wrote:
> Here I attached the cumulative fixes of the patches, new API additions for
> zheap and
> basic outline of the documentation.

I've read the documentation patch while also looking at the code and here
are some comments.

+   Each table is stored as its own physical
relation and so
+   is described by an entry in the pg_class catalog.

I think the "so" in "and so is described by an entry in..." is not necessary.

+   The contents of an table are entirely under the control of its access
method.

"a" table

+   (All the access methods furthermore use the standard page layout
described in
+   .)

Maybe write the two sentences above as:

A table's content is entirely controlled by its access method, although
all access methods use the same standard page layout described in .

+SlotCallbacks_function slot_callbacks;
+
+SnapshotSatisfies_function snapshot_satisfies;
+SnapshotSatisfiesUpdate_function snapshot_satisfiesUpdate;
+SnapshotSatisfiesVacuum_function snapshot_satisfiesVacuum;

Like other functions, how about a one sentence comment for these, like:

/*
 * Function to get an AM-specific set of functions for manipulating
 * TupleTableSlots
 */
SlotCallbacks_function slot_callbacks;

/* AM-specific snapshot visibility determination functions */
SnapshotSatisfies_function snapshot_satisfies;
SnapshotSatisfiesUpdate_function snapshot_satisfiesUpdate;
SnapshotSatisfiesVacuum_function snapshot_satisfiesVacuum;

+TupleFetchFollow_function tuple_fetch_follow;
+
+GetTupleData_function get_tuple_data;

How about removing the empty line so that get_tuple_data can be seen as
part of the group /* Operations on physical tuples */

+RelationVacuum_function relation_vacuum;
+RelationScanAnalyzeNextBlock_function scan_analyze_next_block;
+RelationScanAnalyzeNextTuple_function scan_analyze_next_tuple;
+RelationCopyForCluster_function relation_copy_for_cluster;
+RelationSync_function relation_sync;

Add /* Operations to support VACUUM/ANALYZE */ as a description for this
group?

+BitmapPagescan_function scan_bitmap_pagescan;
+BitmapPagescanNext_function scan_bitmap_pagescan_next;

Add /* Operations to support bitmap scans */ as a description for this group?

+SampleScanNextBlock_function scan_sample_next_block;
+SampleScanNextTuple_function scan_sample_next_tuple;

Add /* Operations to support sampling scans */ as a description for this
group?

+ScanEnd_function scan_end;
+ScanRescan_function scan_rescan;
+ScanUpdateSnapshot_function scan_update_snapshot;

Move these two to be in the /* Operations on relation scans */ group?

+BeginIndexFetchTable_function begin_index_fetch;
+EndIndexFetchTable_function reset_index_fetch;
+EndIndexFetchTable_function end_index_fetch;

Add /* Operations to support index scans */ as a description for this group?

+IndexBuildRangeScan_function index_build_range_scan;
+IndexValidateScan_function index_validate_scan;

Add /* Operations to support index build */ as a description for this group?

+CreateInitFork_function CreateInitFork;

Add /* Function to create an init fork for unlogged tables */?

By the way, I can see the following two in the source code, but not in the
documentation.

EstimateRelSize_function EstimateRelSize;
SetNewFileNode_function SetNewFileNode;


+   The table construction and maintenance functions that an table access
+   method must provide in TableAmRoutine are:

"a" table access method

+  
+
+TupleTableSlotOps *
+slot_callbacks (Relation relation);
+
+   API to access the slot specific methods;
+   Following methods are available;
+   TTSOpsVirtual,
+   TTSOpsHeapTuple,
+   TTSOpsMinimalTuple,
+   TTSOpsBufferTuple,
+  

Unless I'm misunderstanding what the TupleTableSlotOps abstraction is or
its relations to the TableAmRoutine abstraction, I think the text
description could better be written as:

"API to get the slot operations struct for a given table access method"

It's not clear to me why various TTSOps* structs are listed here?  Is the
point that different AMs may choose one of the listed alternatives?  For
example, I see that heap AM implementation returns TTOpsBufferTuple, so it
manipulates slots containing buffered tuples, right?  Other AMs are free
to return any one of these?  For example, some AMs may never use buffer
manager and hence not use TTOpsBufferTuple.  Is that understanding correct?

+  
+
+bool
+snapshot_satisfies (TupleTableSlot *slot, Snapshot snapshot);
+
+   API to check whether the provided slot is visible to the current
+   transaction according the snapshot.
+  

Do you mean:

"API to check whether the tuple contained in the provided slot is
visible"?

+  
+
+Oid
+tuple_insert (Relation rel, TupleTableSlot *slot, CommandId cid,
+  int options, BulkInsertState bistate);
+
+   API to insert the tuple and provide the ItemPointerData
+   where the tuple is successfully inserted.

Re: pg_config wrongly marked as not parallel safe?

[Laurenz Albe]

On Mon, 2018-11-26 at 19:51 -0500, Stephen Frost wrote:
> If a function's results can change across minor or major versions, we
> shouldn't be marking it as immutable because, by definition, it's not
> immutable.
> 
> We, today, have a baked in assumption that any function marked as
> immutable will remain immutable across all major versions that we allow
> indexes to be retained through, which is all of them since about 8.3 at
> this point.
> 
> We absolutely need a policy that if you decide to change the results of
> some immutable function across a major version change, you need to
> consider the results on indexes and possibly write into pg_upgrade
> checks to try and detect any usage of that immutable function.  I hope
> we're in agreement there.

It's hard to make a guarantee that a function will never change in the
future.  What if we fix some rounding or overflow problem in a floating
point function?

If we went that far, hardly any function could be IMMUTABLE.

I think it is best to use IMMUTABLE whenever we don't expect it to
change and it is a function useful for indexes, and if it happens to
change nonetheless, write into the release notes that certain indexes
have to be rebuilt after upgrade.

Of course, there is no problem to mark pg_config as stable, because
there is little chance it will be used in an index anyway.

Yours,
Laurenz Albe

Re: IMMUTABLE and PARALLEL SAFE function markings

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  writes:
>  Tom> Hm. We intentionally allow SQL functions to be marked as less
>  Tom> mutable than their internals, because sometimes that's useful for
>  Tom> tricking the planner. However, IIRC we don't inline when we see
>  Tom> that's the case. Maybe there needs to be a similar restriction for
>  Tom> parallelism markings.

> Well, that doesn't help since not inlining the function doesn't prevent
> it from being called in parallel mode.

In the mutability case, our take on this is "you lied, so it's your
problem if you get the wrong answer".  I'd be inclined to take the
same view in the parallelism case, except for the possibility that
forcing a parallel-unsafe function to be executed in a parallel
worker could lead to a crash.  (This gets back to my point about
implementation details --- we don't know exactly why a function
is marked parallel unsafe, and it could be that it really positively
Doesn't Work in a worker, whereas there's not a similar issue with
respect to not-as-immutable-as-claimed functions.)

However ... if that's possible, then we have a security-ish issue
here, because it's not hard at all to ensure that a SQL function
*won't* be inlined.  So somebody who was intent on crashing a
parallel worker could easily exploit a vulnerable function, inlining
or no: just wrap it in a mislabeled SQL wrapper.

Maybe the only real answer is that we need higher standards about
how badly a parallel-unsafe function can mess up.  Or a lower-level
test that prevents one from being called in a worker, independently
of what the planner thought.

> On second thoughts the problem actually isn't directly to do with
> inlining at all. The problem is that in the presence of polymorphism,
> the function author can't have any confidence in setting any function as
> parallel-safe, since they can't know what the actual functions are that
> will be called at run time.

We've encountered similar issues with respect to the mutability of
datatype I/O functions, and resolved them via a messy compromise:
I/O functions aren't allowed to be volatile, while callers aren't
allowed to assume they're immutable.  Not sure if such a concept
would help here.

regards, tom lane

Re: pg11.1 jit segv

[Justin Pryzby]

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,
Justin

Re: IMMUTABLE and PARALLEL SAFE function markings

[Tom Lane]

Robert Haas  writes:
> On Mon, Nov 26, 2018 at 11:21 PM Tom Lane  wrote:
>> Alternatively, could we postpone the parallelism checks till after
>> function inlining?  Do we even make any before that?

> ... I believe the parallel-safety checks are done very early, and if
> you decide that it's not safe to proceed with parallelism, you can't
> really change your mind later.

What do you consider "very early"?  I do not offhand see a good reason
why we would need to know that before entering query_planner.  Before
that, by definition, we have not made any paths.

regards, tom lane

Re: IMMUTABLE and PARALLEL SAFE function markings

[Robert Haas]

On Mon, Nov 26, 2018 at 11:21 PM Tom Lane  wrote:
> Hm.  We intentionally allow SQL functions to be marked as less mutable
> than their internals, because sometimes that's useful for tricking
> the planner.  However, IIRC we don't inline when we see that's the
> case.  Maybe there needs to be a similar restriction for parallelism
> markings.

Not sure what you have in mind here exactly.  I think that the only
correct thing to do is to mark the parent with the least safe setting
that could apply.  When you inline, you might find out that things are
safer than they initially looked, but I think by then it's too late to
change your mind because ...

> Alternatively, could we postpone the parallelism checks till after
> function inlining?  Do we even make any before that?

... I believe the parallel-safety checks are done very early, and if
you decide that it's not safe to proceed with parallelism, you can't
really change your mind later.

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

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> Hm. We intentionally allow SQL functions to be marked as less
 Tom> mutable than their internals, because sometimes that's useful for
 Tom> tricking the planner. However, IIRC we don't inline when we see
 Tom> that's the case. Maybe there needs to be a similar restriction for
 Tom> parallelism markings.

Well, that doesn't help since not inlining the function doesn't prevent
it from being called in parallel mode.

On second thoughts the problem actually isn't directly to do with
inlining at all. The problem is that in the presence of polymorphism,
the function author can't have any confidence in setting any function as
parallel-safe, since they can't know what the actual functions are that
will be called at run time.

The _nice_ fix for this would be to make it ok to leave inlinable
functions as parallel-unsafe, and have the inlined version checked for
parallel safety. Which would require doing...

 Tom> Alternatively, could we postpone the parallelism checks till after
 Tom> function inlining? Do we even make any before that?

Right now, the parallelism checks are pretty much the very first thing
done on the query, right at the start of standard_planner.

-- 
Andrew (irc:RhodiumToad)

Re: IMMUTABLE and PARALLEL SAFE function markings

[Robert Haas]

On Mon, Nov 26, 2018 at 11:20 PM Andrew Gierth
 wrote:
> But the combination of inlining and polymorphism, in particular, makes
> it impossible for the function author to know this. Take the OP's
> example; it is parallel safe if and only if the selected type's equal
> function is parallel safe - how is the author supposed to know? What if
> the type is one installed later?

I think you have to set it conservatively.  It's easy to construct all
kinds of cases where a function is sometimes parallel-safe and
sometimes not depending on the parameters passed to it, but we don't
have any way to indicate that right now -- and I'm not entirely
convinced that we need one.

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

Re: IMMUTABLE and PARALLEL SAFE function markings

[Tom Lane]

Robert Haas  writes:
> On Mon, Nov 26, 2018 at 8:49 PM Andrew Gierth
>  wrote:
>> I'm a bit more concerned by the fact that inlining the function isn't
>> affecting the parallel safety of the query - the fact that parallel
>> safety is being checked prior to inlining means that if inlining
>> *introduces* a parallel hazard, it will go unnoticed?

> If a function is marked parallel-safe but internally calls
> parallel-restricted or parallel-unsafe functions, it wasn't really
> parallel-safe in the first place.  So I think that if inlining
> introduces a parallel hazard, the user has mislabeled some functions
> and any resulting injury is self-inflicted.

Hm.  We intentionally allow SQL functions to be marked as less mutable
than their internals, because sometimes that's useful for tricking
the planner.  However, IIRC we don't inline when we see that's the
case.  Maybe there needs to be a similar restriction for parallelism
markings.

Alternatively, could we postpone the parallelism checks till after
function inlining?  Do we even make any before that?

regards, tom lane

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andrew Gierth]

> "Robert" == Robert Haas  writes:

 >> I'm a bit more concerned by the fact that inlining the function
 >> isn't affecting the parallel safety of the query - the fact that
 >> parallel safety is being checked prior to inlining means that if
 >> inlining *introduces* a parallel hazard, it will go unnoticed?

 Robert> If a function is marked parallel-safe but internally calls
 Robert> parallel-restricted or parallel-unsafe functions, it wasn't
 Robert> really parallel-safe in the first place. So I think that if
 Robert> inlining introduces a parallel hazard, the user has mislabeled
 Robert> some functions and any resulting injury is self-inflicted.

But the combination of inlining and polymorphism, in particular, makes
it impossible for the function author to know this. Take the OP's
example; it is parallel safe if and only if the selected type's equal
function is parallel safe - how is the author supposed to know? What if
the type is one installed later?

-- 
Andrew (irc:RhodiumToad)

Re: Remove Deprecated Exclusive Backup Mode

[Robert Haas]

On Mon, Nov 26, 2018 at 10:13 PM David Steele  wrote:
> I propose we remove the deprecated exclusive backup mode of
> pg_start_backup()/pg_stop_backup() for Postgres 12.

-1. I don't have a problem with deprecating exclusive backup mode
eventually, but I don't see any good reason to do it this soon.

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.

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.

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

Re: IMMUTABLE and PARALLEL SAFE function markings

[Robert Haas]

On Mon, Nov 26, 2018 at 8:49 PM Andrew Gierth
 wrote:
> I'm a bit more concerned by the fact that inlining the function isn't
> affecting the parallel safety of the query - the fact that parallel
> safety is being checked prior to inlining means that if inlining
> *introduces* a parallel hazard, it will go unnoticed?

If a function is marked parallel-safe but internally calls
parallel-restricted or parallel-unsafe functions, it wasn't really
parallel-safe in the first place.  So I think that if inlining
introduces a parallel hazard, the user has mislabeled some functions
and any resulting injury is self-inflicted.

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

Re: IMMUTABLE and PARALLEL SAFE function markings

[Robert Haas]

On Mon, Nov 26, 2018 at 7:47 PM Vik Fearing  wrote:
> > I'm way less inclined to buy into the idea that it MUST be wrong, though.
> > Immutability is a promise about result stability and lack of side effects,
> > but it is not a promise about implementation details.  There could be an
> > implementation reason not to run something in a parallel worker.  Off the
> > top of my head, a possible example is "it's written in plfoo which hasn't
> > yet been made to work correctly in parallel workers".
>
> Now, see, that is an actual argument for making a difference.  The other
> arguments in this thread were not, so say I.

I agree with you that Tom is the first person to make a real argument
for distinguishing these two things.  And I think his argument is a
good one.  I suspect that there are other cases too.  I don't think
there are all that many cases, but I think they exist.

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

Re: Remove Deprecated Exclusive Backup Mode

[Andres Freund]

Hi,

On 2018-11-27 12:20:13 +0900, Michael Paquier wrote:
> On Mon, Nov 26, 2018 at 10:13:34PM -0500, David Steele wrote:
> > Non-exclusive backups have been available since 9.6 and several third-party
> > solutions support this mode, in addition to pg_basebackup.
> 
> I think that two releases is not actually that much time to just nuke
> the exclusive backup interface.  I would be fine if the docs show the
> deprecation more aggressively using a warning section, and we could add
> an explicit WARNING message about the matter directly when calling
> pg_start_backup and pg_stop_backup.

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.

Greetings,

Andres Freund

Re: Remove Deprecated Exclusive Backup Mode

[Michael Paquier]

On Mon, Nov 26, 2018 at 10:13:34PM -0500, David Steele wrote:
> Non-exclusive backups have been available since 9.6 and several third-party
> solutions support this mode, in addition to pg_basebackup.

I think that two releases is not actually that much time to just nuke
the exclusive backup interface.  I would be fine if the docs show the
deprecation more aggressively using a warning section, and we could add
an explicit WARNING message about the matter directly when calling
pg_start_backup and pg_stop_backup.

My 2c.
--
Michael


signature.asc
Description: PGP signature

Remove Deprecated Exclusive Backup Mode

[David Steele]

Hackers,

I propose we remove the deprecated exclusive backup mode of 
pg_start_backup()/pg_stop_backup() for Postgres 12.


The exclusive backup mode has a serious known issue.  If Postgres 
terminates ungracefully during a backup (due to hardware, kernel, 
Postgres issues, etc.) then Postgres may refuse to restart.


The reason is that the backup_label file will usually reference a 
checkpoint LSN that is older than the WAL available in pg_wal.  Postgres 
does emit a helpful error message while PANIC'ing but that's cold 
comfort to an admin who must manually intervene to get their cluster 
operational again.


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.

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

Re: dsa_allocate() faliure

[Thomas Munro]

On Tue, Nov 27, 2018 at 7:45 AM Alvaro Herrera  wrote:
> On 2018-Nov-26, Jakub Glapa wrote:
> > Justin thanks for the information!
> > I'm running Ubuntu 16.04.
> > I'll try to prepare for the next crash.
> > Couldn't find anything this time.
>
> As I recall, the appport stuff in Ubuntu is terrible ... I've seen it
> take 40 minutes to write the crash dump to disk, during which the
> database was "down".  I don't know why it is so slow (it's a rather
> silly python script that apparently processes the core dump one byte at
> a time, and you can imagine that with a few gigabytes of shared memory
> that takes a while).  Anyway my recommendation was to *remove* that
> stuff from the server and make sure the core file is saved by normal
> means.

Thanks for CC-ing me.  I didn't see this thread earlier because I'm
not subscribed to -performance.  Let's move it over to -hackers since
it looks like it's going to be a debugging exercise.  So, reading
through the thread[1], I think there might be two independent problems
here:

1.  Jakub has a many-partition Parallel Bitmap Heap Scan query that
segfaults when run with max_parallel_workers = 0.  That sounds
suspiciously like an instance of a class of bug we've run into before.
We planned a parallel query, but were unable to launch one due to lack
of DSM slots or process slots, so we run the parallel plan in a kind
of degraded non-parallel mode that needs to cope with various pointers
into shared memory being NULL.  A back trace from a core file should
hopefully make it very obvious what's going on.

2.  The same query when run in real parallel query mode occasionally
reaches an error "dsa_allocate could not find 7 free pages", which
should not happen.  This is on 10.6, so it has the commit "Fix
segment_bins corruption in dsa.c.".

Hmm.  I will see if I can come up with a many-partition torture test
reproducer for this.

[1] 
https://www.postgresql.org/message-id/flat/CAJk1zg10iCNsxFvQ4pgKe1B0rdjNG9iELA7AzLXjXnQm5T%3DKzQ%40mail.gmail.com

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

Re: pg11.1 jit segv

[Andres Freund]

On 2018-11-17 17:37:15 -0600, Justin Pryzby wrote:
> On Fri, Nov 16, 2018 at 10:24:46AM -0600, Justin Pryzby wrote:
> > On Fri, Nov 16, 2018 at 08:38:26AM -0600, Justin Pryzby wrote:
> > > The table is not too special, but was probably ALTERed to add columns a 
> > > good
> > > number of times by one of our processes.  It has ~1100 columns, including
> > > arrays, and some with null_frac=1.  I'm trying to come up with a test case
> > > involving column types and order.
> 
> Try this ?
> 
> SELECT 'DROP TABLE t; CREATE TABLE t (a3 text, a1 int[], 
> '||array_to_string(array_agg('c'||i||' bigint default 0'),',')||'); INSERT 
> INTO t VALUES(0)' FROM generate_series(1,999) i;
> \gexec
> SET jit=on; SET jit_above_cost=0; SELECT a3 FROM t LIMIT 9;
> 
> That's given all sorts of nice errors:
> 
> ERROR:  invalid memory alloc request size 18446744073709551613
> ERROR:  compressed data is corrupted
> 
> And occasionally crashes and/or returns unrelated data:
> 
>  = '0', $21 = '0', $22 = '0', $23 = '0', $24 = '0', $25 = '2741'\x03
>  n 21782 :constvalue 4 [ 0 0 0 0 0 0 0 0 ]}) :location 

Ah, hah. The issue is that t_hoff is larger than 128 here (due to the
size of the NULL bitmap), and apparently getelementptr interprets an
i8 > 128 as a signed integer. Which thus yields a negative offset from
the start of the tuple, which predictably doesn't work great.

v_hoff =
l_load_struct_gep(b, v_tuplep,
  FIELDNO_HEAPTUPLEHEADERDATA_HOFF,
  "t_hoff");
v_tupdata_base =
LLVMBuildGEP(b,
 LLVMBuildBitCast(b,
  v_tuplep,
  l_ptr(LLVMInt8Type()),
  ""),
 &v_hoff, 1,
 "v_tupdata_base");

I'd missed the "These integers are treated as signed values where
relevant." bit in the getelementptr docs
http://llvm.org/docs/LangRef.html#getelementptr-instruction

The fix is easy enough, just adding a
v_hoff = LLVMBuildZExt(b, v_hoff, LLVMInt32Type(), "");
fixes the issue for me.

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.

Greetings,

Andres Freund
diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/llvmjit_deform.c
index 4111bf0a54b..0719675d5b8 100644
--- a/src/backend/jit/llvm/llvmjit_deform.c
+++ b/src/backend/jit/llvm/llvmjit_deform.c
@@ -249,9 +249,11 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 			"maxatt");
 
 	v_hoff =
-		l_load_struct_gep(b, v_tuplep,
-		  FIELDNO_HEAPTUPLEHEADERDATA_HOFF,
-		  "t_hoff");
+		LLVMBuildZExt(b,
+	  l_load_struct_gep(b, v_tuplep,
+		FIELDNO_HEAPTUPLEHEADERDATA_HOFF,
+		""),
+	  LLVMInt32Type(), "t_hoff");
 
 	v_tupdata_base =
 		LLVMBuildGEP(b,

Re: A WalSnd issue related to state WALSNDSTATE_STOPPING

[Michael Paquier]

On Tue, Nov 27, 2018 at 10:07:04AM +0800, Paul Guo wrote:
> Agree. Your code change is safer.

Thanks Paul for double-checking.  I'll come back to this patch in two
days, look at it again and commit it if there are no objections from
others.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Mon, Nov 26, 2018 at 2:08 PM Masahiko Sawada  wrote:
>
> On Sun, Nov 25, 2018 at 2:35 PM Amit Kapila  wrote:
> >
> > On Sat, Nov 24, 2018 at 5:47 PM Amit Kapila  wrote:
> > > On Tue, Oct 30, 2018 at 2:04 PM Masahiko Sawada  
> > > wrote:
> > > >
> > >
>
> Thank you for the comment.
>
> > > I could see that you have put a lot of effort on this patch and still
> > > we are not able to make much progress mainly I guess because of
> > > relation extension lock problem.  I think we can park that problem for
> > > some time (as already we have invested quite some time on it), discuss
> > > a bit about actual parallel vacuum patch and then come back to it.
> > >
> >
> > Today, I was reading this and previous related thread [1] and it seems
> > to me multiple people Andres [2], Simon [3] have pointed out that
> > parallelization for index portion is more valuable.  Also, some of the
> > results [4] indicate the same.  Now, when there are no indexes,
> > parallelizing heap scans also have benefit, but I think in practice we
> > will see more cases where the user wants to vacuum tables with
> > indexes.  So how about if we break this problem in the following way
> > where each piece give the benefit of its own:
> > (a) Parallelize index scans wherein the workers will be launched only
> > to vacuum indexes.  Only one worker per index will be spawned.
> > (b) Parallelize per-index vacuum.  Each index can be vacuumed by
> > multiple workers.
> > (c) Parallelize heap scans where multiple workers will scan the heap,
> > collect dead TIDs and then launch multiple workers for indexes.
> >
> > I think if we break this problem into multiple patches, it will reduce
> > the scope of each patch and help us in making progress.   Now, it's
> > been more than 2 years that we are trying to solve this problem, but
> > still didn't make much progress.  I understand there are various
> > genuine reasons and all of that work will help us in solving all the
> > problems in this area.  How about if we first target problem (a) and
> > once we are done with that we can see which of (b) or (c) we want to
> > do first?
>
> Thank you for suggestion. It seems good to me. We would get a nice
> performance scalability even by only (a), and vacuum will get more
> powerful by (b) or (c). Also, (a) would not require to resovle the
> relation extension lock issue IIUC.
>

Yes, I also think so.  We do acquire 'relation extension lock' during
index vacuum, but as part of (a), we are talking one worker per-index,
so there shouldn't be a problem with respect to deadlocks.

> I'll change the patch and submit
> to the next CF.
>

Okay.

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

Re: A WalSnd issue related to state WALSNDSTATE_STOPPING

[Paul Guo]

>
> On Thu, Nov 22, 2018 at 06:31:04PM +0800, Paul Guo wrote:
> > Yes, please see the attached patch.
>
> Thanks, I have reviewed your patch, and could not resist to change
> SyncRepReleaseWaiters() on top of the rest with a comment to not be
> confused about the WAL sender states allowed to release waiters.
>
> The previous experience is proving that it seems unwise to rely on the
> order of the elements in WalSndState, so I have tweaked things so as the
> state is listed for all the code paths involved.
>
> Paul, what do you think?
> --
> Michael
>

Agree. Your code change is safer.

Re: Pluggable Storage - Andres's take

[Andres Freund]

Hi,

FWIW, now that oids are removed, and the tuple table slot abstraction
got in, I'm working on rebasing the pluggable storage patchset ontop of
that.


On 2018-11-27 12:48:36 +1100, Haribabu Kommi wrote:
> On Thu, Nov 22, 2018 at 1:12 PM Asim R P  wrote:
> 
> > Ashwin (copied) and I got a chance to go through the latest code from
> > Andres' github repository.  We would like to share some
> > comments/quesitons:
> >
> 
> Thanks for the review.
> 
> 
> > The TupleTableSlot argument is well suited for row-oriented storage.
> > For a column-oriented storage engine, a projection list indicating the
> > columns to be scanned may be necessary.  Is it possible to share this
> > information with current interface?
> >
> 
> Currently all the interfaces are designed for row-oriented storage, as you
> said we need a new API for projection list. The current patch set itself
> is big and it needs to stabilized and then in the next set of the patches,
> those new API's will be added that will be useful for columnar storage.

Precisely.


> > TupleDelete_function() accepts changingPart as a parameter to indicate
> > if this deletion is part of a movement from one partition to another.
> > Partitioning is a higher level abstraction as compared to storage.
> > Ideally, storage layer should have no knowledge of partitioning. The
> > tuple delete API should not accept any parameter related to
> > partitioning.
> >
> 
> Thanks for your point, will look into it in how to change extract it.

I don't think that's actually a problem. The changingPart parameter is
just a marker that the deletion is part of moving a tuple across
partitions. For heap and everythign compatible that's used to include
information to the tuple that concurrent modifications to the tuple
should error out when reaching such a tuple via EPQ.


> Andres, as the tupletableslot changes are committed, do you want me to
> share the rebased pluggable storage patch? you already working on it?

Working on it.


Greetings,

Andres Freund

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

[Haribabu Kommi]

On Fri, Nov 23, 2018 at 6:39 PM Thomas Munro 
wrote:

> On Mon, Nov 19, 2018 at 5:24 PM Kyotaro HORIGUCHI
>  wrote:
> > At Sat, 17 Nov 2018 11:15:54 -0300, Alvaro Herrera <
> alvhe...@2ndquadrant.com> wrote in
> <20181117141554.4dkx2u4j6md3bqdh@alvherre.pgsql>
> > > Is this patch committable now?
> >
> > I don't think so. We should make a decision on a point.
> >
> > I was a bit confused (sorry) but IIUIC Haribabu suggested that
> > the RBM_ZERO_ON_ERROR case should be included to read (not just
> > ignored). I'm on it.  I think that RPM_ZERO_* other than ON_ERROR
> > cases could be a kind of hit but I don't insist on it.
> >
> > So, I think we should decide on at least the ON_ERROR case before
> > this becomes commttable.
>
> Agreed, RBM_ZERO_ON_ERROR needs to be counted as a read.  Here's a new
> version like that.
>
> > The another counter could be another issue. I don't object to add
> > the counter but I'm not sure what is the suitable name.
>
> I think that might be interesting information in the future, but let's
> consider that for a later patch.
>

Thanks for the updated patch. It looks good.
I marked it in the commitfest as ready for committer.

Regards,
Haribabu Kommi
Fujitsu Australia

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andrew Gierth]

I'm a bit more concerned by the fact that inlining the function isn't
affecting the parallel safety of the query - the fact that parallel
safety is being checked prior to inlining means that if inlining
*introduces* a parallel hazard, it will go unnoticed?

-- 
Andrew (irc:RhodiumToad)

Re: Pluggable Storage - Andres's take

[Haribabu Kommi]

On Thu, Nov 22, 2018 at 1:12 PM Asim R P  wrote:

> Ashwin (copied) and I got a chance to go through the latest code from
> Andres' github repository.  We would like to share some
> comments/quesitons:
>

Thanks for the review.


> The TupleTableSlot argument is well suited for row-oriented storage.
> For a column-oriented storage engine, a projection list indicating the
> columns to be scanned may be necessary.  Is it possible to share this
> information with current interface?
>

Currently all the interfaces are designed for row-oriented storage, as you
said we need a new API for projection list. The current patch set itself
is big and it needs to stabilized and then in the next set of the patches,
those new API's will be added that will be useful for columnar storage.


> We realized that DDLs such as heap_create_with_catalog() are not
> generalized.  Haribabu's latest patch that adds
> SetNewFileNode_function() and CreateInitFort_function() is a step
> towards this end.  However, the current API assumes that the storage
> engine uses relation forks.  Isn't that too restrictive?
>

Current set of API has many assumptions and uses the existing framework.
Thanks for your point, will check it how to enhance it.


> TupleDelete_function() accepts changingPart as a parameter to indicate
> if this deletion is part of a movement from one partition to another.
> Partitioning is a higher level abstraction as compared to storage.
> Ideally, storage layer should have no knowledge of partitioning. The
> tuple delete API should not accept any parameter related to
> partitioning.
>

Thanks for your point, will look into it in how to change extract it.


> The API needs to be more accommodating towards block sizes used in
> storage engines.  Currently, the same block size as heap seems to be
> assumed, as evident from the type of some members of generic scan
> object:
>
> typedef struct TableScanDescData
> {
>   /* state set up at initscan time */
>   BlockNumber rs_nblocks; /* total number of blocks in rel */
>   BlockNumber rs_startblock;  /* block # to start at */
>   BlockNumber rs_numblocks;   /* max number of blocks to scan */
>   /* rs_numblocks is usually InvalidBlockNumber, meaning "scan whole rel"
> */
>   boolrs_syncscan;/* report location to syncscan logic? */
> }   TableScanDescData;
>
> Using bytes to represent this information would be more generic. E.g.
> rs_startlocation as bytes/offset instead of rs_startblock and so on.
>

I doubt that this may not be the only one that needs a change to support
different block sizes for different storage interfaces. Thanks for your
point,
but definitely this can be taken care in the next set of patches.

Andres, as the tupletableslot changes are committed, do you want me to
share the rebased pluggable storage patch? you already working on it?

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Handling of REGRESS_OPTS in MSVC for regression tests

[Michael Paquier]

On Mon, Nov 26, 2018 at 12:41:21PM -0500, Andrew Dunstan wrote:
> It's not the buildfarm that is broken. Both contribcheck() and
> modulescheck() in vcregress.pl run in installcheck mode, i.e. with an
> already running database. That makes the tests run faster, because setting
> up and breaking down instances is expensive, especially on Windows. We've
> got far too profligate with that in recent years, to the extent that some of
> my Windows buildfarm animals take many hours to complete full runs these
> days.

I have noticed the performance impact when switching to check for those
targets.  It was bad.

> Note that TAP tests are not a problem here. It's up to the TAP scripts to
> set up and break down postgres instances they need, with whatever config
> options are required, such as shared preload libraries. It's only modules
> using pg_regress that could be a problem.

Indeed.

> A simple way to proceed might be to have vcregress.pl honor the
> NO_INSTALLCHECK flag. Currently this is only used by pg_stat_statements, but
> we could add it anywhere required. In vcregress.pl, I would probably do this
> by having fetchTests() check for the flag and return an empty set of tests
> if it's found, and in turn have subdircheck() do nothing if it has an empty
> set of tests.

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?
--
Michael
diff --git a/src/test/modules/commit_ts/Makefile b/src/test/modules/commit_ts/Makefile
index 6d4f3be358..6a9c3971fb 100644
--- a/src/test/modules/commit_ts/Makefile
+++ b/src/test/modules/commit_ts/Makefile
@@ -2,6 +2,9 @@
 
 REGRESS = commit_timestamp
 REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
+# Disabled because these tests require "track_commit_timestamp = on",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/src/test/modules/test_rls_hooks/Makefile b/src/test/modules/test_rls_hooks/Makefile
index 6b772c4db1..284fdaf095 100644
--- a/src/test/modules/test_rls_hooks/Makefile
+++ b/src/test/modules/test_rls_hooks/Makefile
@@ -9,6 +9,9 @@ EXTENSION = test_rls_hooks
 
 REGRESS = test_rls_hooks
 REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
+# Disabled because these tests require "shared_preload_libraries=test_rls_hooks",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 599b521014..4018313bf2 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -361,6 +361,10 @@ sub plcheck
 		{
 			@lang_args = ();
 		}
+
+		# Move on if no tests are listed.
+		next if (scalar @tests == 0);
+
 		print
 		  "\n";
 		print "Checking $lang\n";
@@ -391,6 +395,14 @@ sub subdircheck
 
 	chdir $module;
 	my @tests = fetchTests();
+
+	# Leave if no tests are listed in the module.
+	if (scalar @tests == 0)
+	{
+		chdir "..";
+		return;
+	}
+
 	my @opts  = fetchRegressOpts();
 
 	# Special processing for python transform modules, see their respective
@@ -638,6 +650,8 @@ sub fetchRegressOpts
 	return @opts;
 }
 
+# Fetch the list of tests by parsing a module's Makefile.  An empty
+# list is returned if the module does not need to run anything.
 sub fetchTests
 {
 
@@ -651,6 +665,14 @@ sub fetchTests
 	my $t = "";
 
 	$m =~ s{\\\r?\n}{}g;
+
+	# A module specifying NO_INSTALLCHECK does not support installcheck,
+	# so bypass its run by returning an empty set of tests.
+	if ($m =~ /^\s*NO_INSTALLCHECK\s*=\s*\S+/m)
+	{
+		return ();
+	}
+
 	if ($m =~ /^REGRESS\s*=\s*(.*)$/gm)
 	{
 		$t = $1;


signature.asc
Description: PGP signature

Re: reg* checks in pg_upgrade are out of date

[Andres Freund]

On 2018-11-22 08:49:23 -0500, Andrew Dunstan wrote:
> 
> On 11/21/18 7:12 PM, Andres Freund wrote:
> > Hi,
> > 
> > It seems the list of reg* types and the check for them in pg_upgrade
> > have gone out of sync. We have the following reg* types:
> > 
> >   SELECT typname FROM pg_type WHERE typname LIKE 'reg%' order by typname;
> > ┌───┐
> > │typname│
> > ├───┤
> > │ regclass  │
> > │ regconfig │
> > │ regdictionary │
> > │ regnamespace  │
> > │ regoper   │
> > │ regoperator   │
> > │ regproc   │
> > │ regprocedure  │
> > │ regrole   │
> > │ regtype   │
> > └───┘
> > (10 rows)
> > 
> > but pg_upgrade doesn't consider all of them:
> > 
> >  /*
> >   * While several relkinds don't store any data, e.g. views, they 
> > can
> >   * be used to define data types of other columns, so we check all
> >   * relkinds.
> >   */
> >  res = executeQueryOrDie(conn,
> >  "SELECT n.nspname, c.relname, a.attname "
> >  "FROM   pg_catalog.pg_class c, "
> >  "   pg_catalog.pg_namespace n, "
> >  "   pg_catalog.pg_attribute a "
> >  "WHERE  c.oid = a.attrelid AND "
> >  "   NOT a.attisdropped AND "
> >  "   a.atttypid IN ( "
> >  "   
> > 'pg_catalog.regproc'::pg_catalog.regtype, "
> >  "   
> > 'pg_catalog.regprocedure'::pg_catalog.regtype, "
> >  "   
> > 'pg_catalog.regoper'::pg_catalog.regtype, "
> >  "   
> > 'pg_catalog.regoperator'::pg_catalog.regtype, "
> >  /* regclass.oid is preserved, so 'regclass' is OK */
> >  /* regtype.oid is preserved, so 'regtype' is OK */
> >  "   
> > 'pg_catalog.regconfig'::pg_catalog.regtype, "
> >  "   
> > 'pg_catalog.regdictionary'::pg_catalog.regtype) AND "
> >  "   c.relnamespace = n.oid AND "
> >  "   n.nspname NOT IN ('pg_catalog', 
> > 'information_schema')");
> > 
> > (I don't get the order here btw)
> > 
> > ISTM when regrole and regnamespace were added, pg_upgrade wasn't
> > considered. It turns out that regrole is safe, because we preserve user
> > oids, but regnamespace isn't afaict.  I don't think it's extremely
> > likely that users store such reg* columns in tables, but we probably
> > still should fix this.
> > 
> 
> yeah, I think you're right, both about the need to fix it and the
> unlikelihood of it occurring in the wild.

I've done so, and backpatched to 9.5, where these types where added.

Greetings,

Andres Freund

Re: pg_config wrongly marked as not parallel safe?

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-26 19:34:03 -0500, Stephen Frost wrote:
> > These two things seem entirely independent in my view, so I'm really not
> > sure that I'm following what you're getting at.
> 
> All I said is that I don't think it's a reasonable policy to mark all
> functions that potentially could change across major versions as
> immutable.  I've no problem with changing pg_config in particular,
> especially as it - as has been pointed - clearly can change in minor
> versions / recompiles.

I'm...  not following.

If a function's results can change across minor or major versions, we
shouldn't be marking it as immutable because, by definition, it's not
immutable.

We, today, have a baked in assumption that any function marked as
immutable will remain immutable across all major versions that we allow
indexes to be retained through, which is all of them since about 8.3 at
this point.

We absolutely need a policy that if you decide to change the results of
some immutable function across a major version change, you need to
consider the results on indexes and possibly write into pg_upgrade
checks to try and detect any usage of that immutable function.  I hope
we're in agreement there.

In other words, maybe it isn't a sealed-in-concrete policy that you
can't go around changing what an immutable function returns, but you
certainly better have a really good justification for it, and write
all of the code to detect any cases where that could cause incorrect
results from the database, including pg_upgrade throwing a big error if
it finds any indexes (or maybe even functions...) using it, and even
then I'm pretty darn skeptical about accepting it.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 27/11/2018 01:40, Tom Lane wrote:
> Vik Fearing  writes:
>> On 27/11/2018 01:13, Stephen Frost wrote:
>>> Parallel safe functions should be marked as such.  Immutable functions
>>> should be marked as such.  We should not assume that one implies the
>>> other, nor should we operate as if they do.
> 
>> Yes we should!  Unless you can produce a case where an immutable
>> function is not parallel safe.
> 
> As far as that goes, I agree with the idea of adding an oprsanity test
> that shows any built-in functions that are immutable but not parallel
> safe, on the grounds Stephen mentioned: it's probably a mistake, and
> if it isn't, we can add that function to the expected output.
> 
> I'm way less inclined to buy into the idea that it MUST be wrong, though.
> Immutability is a promise about result stability and lack of side effects,
> but it is not a promise about implementation details.  There could be an
> implementation reason not to run something in a parallel worker.  Off the
> top of my head, a possible example is "it's written in plfoo which hasn't
> yet been made to work correctly in parallel workers".

Now, see, that is an actual argument for making a difference.  The other
arguments in this thread were not, so say I.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Re: pg_config wrongly marked as not parallel safe?

[Andres Freund]

On 2018-11-26 19:34:03 -0500, Stephen Frost wrote:
> These two things seem entirely independent in my view, so I'm really not
> sure that I'm following what you're getting at.

All I said is that I don't think it's a reasonable policy to mark all
functions that potentially could change across major versions as
immutable.  I've no problem with changing pg_config in particular,
especially as it - as has been pointed - clearly can change in minor
versions / recompiles.

Re: IMMUTABLE and PARALLEL SAFE function markings

[Tom Lane]

Vik Fearing  writes:
> On 27/11/2018 01:13, Stephen Frost wrote:
>> Parallel safe functions should be marked as such.  Immutable functions
>> should be marked as such.  We should not assume that one implies the
>> other, nor should we operate as if they do.

> Yes we should!  Unless you can produce a case where an immutable
> function is not parallel safe.

As far as that goes, I agree with the idea of adding an oprsanity test
that shows any built-in functions that are immutable but not parallel
safe, on the grounds Stephen mentioned: it's probably a mistake, and
if it isn't, we can add that function to the expected output.

I'm way less inclined to buy into the idea that it MUST be wrong, though.
Immutability is a promise about result stability and lack of side effects,
but it is not a promise about implementation details.  There could be an
implementation reason not to run something in a parallel worker.  Off the
top of my head, a possible example is "it's written in plfoo which hasn't
yet been made to work correctly in parallel workers".

regards, tom lane

Re: IMMUTABLE and PARALLEL SAFE function markings

[Stephen Frost]

Greetings,

* Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
> On 27/11/2018 01:13, Stephen Frost wrote:
> > Parallel safe functions should be marked as such.  Immutable functions
> > should be marked as such.  We should not assume that one implies the
> > other, nor should we operate as if they do.
> 
> Yes we should!  Unless you can produce a case where an immutable
> function is not parallel safe.

What's the advantage of running a function that isn't marked as parallel
safe in a parallel worker because it's marked as immutable?

Seems like the only thing we're doing is making assumptions about what
the user meant when they've clearly told us something different and that
just strikes me as both a bad idea and an unnecessary complication of
the code.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Tom Lane]

FWIW, I'm inclined to think that pg_config should be marked as stable
not immutable.  Aside from the minor-version-upgrade issue, what if
you install new binaries that are the same version but built with
different configure flags?

The pg_config outputs are roughly as stable as initdb-inserted
catalog data, and we've never judged that functions that return
catalog data can be marked immutable.

There seems no reason it can't be parallel safe, though, since
worker processes should get the same answers as their parent.

regards, tom lane

Re: pg_config wrongly marked as not parallel safe?

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-26 19:16:00 -0500, Stephen Frost wrote:
> > * Andres Freund (and...@anarazel.de) wrote:
> > > On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
> > > > So the correct answer is probably to mark pg_config() stable, but it
> > > > still seems to be parallel safe to me.
> > > 
> > > I don't think we should consider immutability to mean anything across
> > > major versions. What'd be helped by doing that? We'd have to rule out
> > > any behaviour change to any immutable function for that to make
> > > sense. Including making an immutable function not immutable anymore.
> > 
> > Then we have to require that all indexes built with immutable functions
> > be rebuilt when someone does a pg_upgrade from one major version to the
> > next.
> 
> Happened a couple times. The harm from overaggressively removing
> immutability, and thus not even being able to add such indexes and
> obviously worse query plans, seems much bigger than avoiding the need to
> rebuild indexes in some cases.

These two things seem entirely independent in my view, so I'm really not
sure that I'm following what you're getting at.

We have required specific kinds of indexes to be rebuilt across major
version upgrades, yes, but those have almost uniformly been cases where
the index was very rarely used or only used in some corner cases.

I certainly do not believe we should invalidate all indexes built with
immutable functions across major versions upgrades.  Perhaps in some
cases when we've decided that an immutable function today should work in
some different way in a new version then we should complain about or
invalidate those indexes, but that's a special case.  Taken to an
extreme, you could argue that partial indexes should also be rebuilt on
major version upgrades, in case we decide that 1 is suddently less than
2, but instead of baking that assumption in, let's just deal with those
cases when they come up and write logic into pg_upgrade to detect such
usages and make the user aware of them.

If you feel that immutability is being overaggressively removed from
something, that's fair, but should also be addressed on a case-by-case
basis.  In this particular case, I don't see any concern that we're
going to break user's indexes by changing pg_config() to be stable
instead of immutable, and I don't really think we even need to write
logic into pg_upgrade for it- unless it could be generalized to detect
such cases between versions, in which case I'd be all for that in case
we have other such changes.  Not sure it's really worth it though, we
don't have that happen all that often.

Thanks!

Stpehen


signature.asc
Description: PGP signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andres Freund]

Hi,

On 2018-11-27 01:27:41 +0100, Vik Fearing wrote:
> On 27/11/2018 01:13, Stephen Frost wrote:
> > Parallel safe functions should be marked as such.  Immutable functions
> > should be marked as such.  We should not assume that one implies the
> > other, nor should we operate as if they do.
> 
> Yes we should!  Unless you can produce a case where an immutable
> function is not parallel safe.

Well, then write a patch that documents that behaviour, automatically
sets proparallel accordingly, and defines an escape hatch for when the
user actually meant unsafe, not just "unsafe maybe". Just saying "we
should" doesn't go far actually convincing anybody.

Greetings,

Andres Freund

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 27/11/2018 01:13, Stephen Frost wrote:
> Parallel safe functions should be marked as such.  Immutable functions
> should be marked as such.  We should not assume that one implies the
> other, nor should we operate as if they do.

Yes we should!  Unless you can produce a case where an immutable
function is not parallel safe.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support



signature.asc
Description: OpenPGP digital signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 27/11/2018 01:14, Stephen Frost wrote:
> Greetings,
> 
> * Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
>> On 27/11/2018 01:10, Stephen Frost wrote:
>>> * Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
 On 27/11/2018 01:05, Stephen Frost wrote:
> That said, I do *not* think we should make any assumptions here- users
> incorrectly mark things all the time but we shouldn't encourage that and
> we shouldn't assume that functions marked as immutable are parallel
> safe.

 Does that mean we also shouldn't assume that functions marked as
 immutable are index safe?
>>>
>>> We've got an index safe flag?
>>
>> Yes.  It's called provolatile='i'.
> 
> ... and we complain if someone tries to use a provolatile <> 'i'
> function directly in an index, so not sure what you're getting at here?

I'm getting at we should do the same for parallel safety checks.

If it's immutable, it's safe.
If it's not immutable, check if it's safe.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support



signature.asc
Description: OpenPGP digital signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-26 19:13:17 -0500, Stephen Frost wrote:
> > * Andres Freund (and...@anarazel.de) wrote:
> > > On 2018-11-26 19:05:02 -0500, Stephen Frost wrote:
> > > > Agreed, but I could see us having a regression test which complains if
> > > > it finds any which are marked as immutable but aren't parallel safe.
> > > 
> > > That doesn't help if a user writes a query to review the not parallel
> > > safe functions in their installation.
> > 
> > I'm really not sure what you're getting at here..?
> 
> You replied to me saying it'd be a bad idea to infer parallel safety
> from immutability:

I don't think we should programatically assume it, but given that it's
very likely the case, we should have a check (as you suggested in the
other thread) that makes PG developers at least think about it.

> > > > Surely a simple rule could be made that provolatile='i' trumps
> > > > proparallel.  No need to make them agree.
> > > > [...]
> > > 
> > > I think it'd be entirely unacceptable that
> > >   SELECT * FROM pg_proc WHERE proparallel = 's'
> > > wouldn't actually return all the functions that are parallel safe.
> > 
> > Agreed, but I could see us having a regression test which complains if
> > it finds any which are marked as immutable but aren't parallel safe.
> 
> I'm saying that that doesn't address my concern.

Then I'm unclear as to what the concern here is..?

> > Parallel safe functions should be marked as such.  Immutable functions
> > should be marked as such.  We should not assume that one implies the
> > other, nor should we operate as if they do.
> > 
> > My suggestion for a regression test was to make PG developers really
> > think about if their new immutable functions should also be marked as
> > parallel safe, in the event that they forget to mark it as such.  If
> > it's really immutable and not parallel safe, then they need to adjust
> > the expected regression test output (and we can all see it...).
> 
> See 
> http://archives.postgresql.org/message-id/20181126234521.rh3grz7aavx2ubjv%40alap3.anarazel.de

Right, so you're also suggestion we have a regression test for it..

I'm starting to wonder if maybe we're in violent agreement here..?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: pg_config wrongly marked as not parallel safe?

[Andres Freund]

Hi,

On 2018-11-26 19:16:00 -0500, Stephen Frost wrote:
> Greetings,
> 
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
> > > So the correct answer is probably to mark pg_config() stable, but it
> > > still seems to be parallel safe to me.
> > 
> > I don't think we should consider immutability to mean anything across
> > major versions. What'd be helped by doing that? We'd have to rule out
> > any behaviour change to any immutable function for that to make
> > sense. Including making an immutable function not immutable anymore.
> 
> Then we have to require that all indexes built with immutable functions
> be rebuilt when someone does a pg_upgrade from one major version to the
> next.

Happened a couple times. The harm from overaggressively removing
immutability, and thus not even being able to add such indexes and
obviously worse query plans, seems much bigger than avoiding the need to
rebuild indexes in some cases.


> Not to mention that the issue at hand isn't a major version upgrade
> anyway, it's a minor version change...

Yea, that obviously makes this different.  Apparently the old versioning
scheme is stuck in my head on a deep enough level...

Greetings,

Andres Freund

Re: pg_config wrongly marked as not parallel safe?

[Andres Freund]

On 2018-11-26 19:14:24 -0500, Joe Conway wrote:
> On 11/26/18 7:08 PM, Andres Freund wrote:
> > On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
> >> Not intentional. Though, sitting here chatting with Stephen about it, I
> >> am now wondering if pg_config() should actually be marked immutable:
> >>
> >> select * from pg_config() where name = 'VERSION';
> >>   name   | setting
> >> -+-
> >>  VERSION | PostgreSQL 10.5
> >> (1 row)
> >>
> >> [...upgrade the postgres binaries...]
> >>
> >> select * from pg_config() where name = 'VERSION';
> >>   name   | setting
> >> -+-
> >>  VERSION | PostgreSQL 10.6
> >> (1 row)
> >>
> >> So the correct answer is probably to mark pg_config() stable, but it
> >> still seems to be parallel safe to me.
> > 
> > I don't think we should consider immutability to mean anything across
> > major versions. What'd be helped by doing that? We'd have to rule out
> > any behaviour change to any immutable function for that to make
> > sense. Including making an immutable function not immutable anymore.
> 
> Umm, this is a minor version not major.

Oops.

Greetings,

Andres Freund

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andres Freund]

On 2018-11-26 19:13:17 -0500, Stephen Frost wrote:
> Greetings,
> 
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2018-11-26 19:05:02 -0500, Stephen Frost wrote:
> > > Agreed, but I could see us having a regression test which complains if
> > > it finds any which are marked as immutable but aren't parallel safe.
> > 
> > That doesn't help if a user writes a query to review the not parallel
> > safe functions in their installation.
> 
> I'm really not sure what you're getting at here..?

You replied to me saying it'd be a bad idea to infer parallel safety
from immutability:

> > > Surely a simple rule could be made that provolatile='i' trumps
> > > proparallel.  No need to make them agree.
> > > [...]
> > 
> > I think it'd be entirely unacceptable that
> >   SELECT * FROM pg_proc WHERE proparallel = 's'
> > wouldn't actually return all the functions that are parallel safe.
> 
> Agreed, but I could see us having a regression test which complains if
> it finds any which are marked as immutable but aren't parallel safe.

I'm saying that that doesn't address my concern.


> Parallel safe functions should be marked as such.  Immutable functions
> should be marked as such.  We should not assume that one implies the
> other, nor should we operate as if they do.
> 
> My suggestion for a regression test was to make PG developers really
> think about if their new immutable functions should also be marked as
> parallel safe, in the event that they forget to mark it as such.  If
> it's really immutable and not parallel safe, then they need to adjust
> the expected regression test output (and we can all see it...).

See 
http://archives.postgresql.org/message-id/20181126234521.rh3grz7aavx2ubjv%40alap3.anarazel.de

Greetings,

Andres Freund

Re: pg_config wrongly marked as not parallel safe?

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
> > So the correct answer is probably to mark pg_config() stable, but it
> > still seems to be parallel safe to me.
> 
> I don't think we should consider immutability to mean anything across
> major versions. What'd be helped by doing that? We'd have to rule out
> any behaviour change to any immutable function for that to make
> sense. Including making an immutable function not immutable anymore.

Then we have to require that all indexes built with immutable functions
be rebuilt when someone does a pg_upgrade from one major version to the
next.

Not to mention that the issue at hand isn't a major version upgrade
anyway, it's a minor version change...

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: pg_config wrongly marked as not parallel safe?

[Joe Conway]

On 11/26/18 7:08 PM, Andres Freund wrote:
> On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
>> Not intentional. Though, sitting here chatting with Stephen about it, I
>> am now wondering if pg_config() should actually be marked immutable:
>>
>> select * from pg_config() where name = 'VERSION';
>>   name   | setting
>> -+-
>>  VERSION | PostgreSQL 10.5
>> (1 row)
>>
>> [...upgrade the postgres binaries...]
>>
>> select * from pg_config() where name = 'VERSION';
>>   name   | setting
>> -+-
>>  VERSION | PostgreSQL 10.6
>> (1 row)
>>
>> So the correct answer is probably to mark pg_config() stable, but it
>> still seems to be parallel safe to me.
> 
> I don't think we should consider immutability to mean anything across
> major versions. What'd be helped by doing that? We'd have to rule out
> any behaviour change to any immutable function for that to make
> sense. Including making an immutable function not immutable anymore.

Umm, this is a minor version not major.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: IMMUTABLE and PARALLEL SAFE function markings

[Stephen Frost]

Greetings,

* Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
> On 27/11/2018 01:10, Stephen Frost wrote:
> > * Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
> >> On 27/11/2018 01:05, Stephen Frost wrote:
> >>> That said, I do *not* think we should make any assumptions here- users
> >>> incorrectly mark things all the time but we shouldn't encourage that and
> >>> we shouldn't assume that functions marked as immutable are parallel
> >>> safe.
> >>
> >> Does that mean we also shouldn't assume that functions marked as
> >> immutable are index safe?
> > 
> > We've got an index safe flag?
> 
> Yes.  It's called provolatile='i'.

... and we complain if someone tries to use a provolatile <> 'i'
function directly in an index, so not sure what you're getting at here?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-26 19:05:02 -0500, Stephen Frost wrote:
> > Agreed, but I could see us having a regression test which complains if
> > it finds any which are marked as immutable but aren't parallel safe.
> 
> That doesn't help if a user writes a query to review the not parallel
> safe functions in their installation.

I'm really not sure what you're getting at here..?

Parallel safe functions should be marked as such.  Immutable functions
should be marked as such.  We should not assume that one implies the
other, nor should we operate as if they do.

My suggestion for a regression test was to make PG developers really
think about if their new immutable functions should also be marked as
parallel safe, in the event that they forget to mark it as such.  If
it's really immutable and not parallel safe, then they need to adjust
the expected regression test output (and we can all see it...).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 27/11/2018 01:10, Stephen Frost wrote:
> Greetings,
> 
> * Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
>> On 27/11/2018 01:05, Stephen Frost wrote:
>>> That said, I do *not* think we should make any assumptions here- users
>>> incorrectly mark things all the time but we shouldn't encourage that and
>>> we shouldn't assume that functions marked as immutable are parallel
>>> safe.
>>
>> Does that mean we also shouldn't assume that functions marked as
>> immutable are index safe?
> 
> We've got an index safe flag?

Yes.  It's called provolatile='i'.

> That's news to me.
> 
> Thanks!

You're welcome.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support



signature.asc
Description: OpenPGP digital signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Stephen Frost]

Greetings,

* Vik Fearing (vik.fear...@2ndquadrant.com) wrote:
> On 27/11/2018 01:05, Stephen Frost wrote:
> > That said, I do *not* think we should make any assumptions here- users
> > incorrectly mark things all the time but we shouldn't encourage that and
> > we shouldn't assume that functions marked as immutable are parallel
> > safe.
> 
> Does that mean we also shouldn't assume that functions marked as
> immutable are index safe?

We've got an index safe flag?

That's news to me.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andres Freund]

Hi,

On 2018-11-26 19:05:02 -0500, Stephen Frost wrote:
> Agreed, but I could see us having a regression test which complains if
> it finds any which are marked as immutable but aren't parallel safe.

That doesn't help if a user writes a query to review the not parallel
safe functions in their installation.

Greetings,

Andres Freund

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 27/11/2018 01:05, Stephen Frost wrote:
> That said, I do *not* think we should make any assumptions here- users
> incorrectly mark things all the time but we shouldn't encourage that and
> we shouldn't assume that functions marked as immutable are parallel
> safe.

Does that mean we also shouldn't assume that functions marked as
immutable are index safe?

User error is user error.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support



signature.asc
Description: OpenPGP digital signature

Re: pg_config wrongly marked as not parallel safe?

[Andres Freund]

On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
> On 11/26/18 6:45 PM, Andres Freund wrote:
> > Hi,
> > 
> > Triggered by the thread at [1] I looked for functions marked as
> > immutable but not parallel safe.
> > 
> > postgres[19492][1]=# SELECT oid::regprocedure, provolatile, proparallel 
> > FROM pg_proc WHERE provolatile = 'i' AND proparallel != 's';
> > ┌─┬─┬─┐
> > │ oid │ provolatile │ proparallel │
> > ├─┼─┼─┤
> > │ pg_config() │ i   │ r   │
> > └─┴─┴─┘
> > (1 row)
> > 
> > # pg_config
> > { oid => '3400', descr => 'pg_config binary as a function',
> >   proname => 'pg_config', prorows => '23', proretset => 't', proparallel => 
> > 'r',
> >   prorettype => 'record', proargtypes => '', proallargtypes => 
> > '{text,text}',
> >   proargmodes => '{o,o}', proargnames => '{name,setting}',
> >   prosrc => 'pg_config' },
> > 
> > so that function is marked as immutable but not parallel safe, without
> > an explanation for that odd combination.
> > 
> > Now obviously I don't think it practially matters for pg_config(), but
> > it seems unnecessarily confusing as a general matter.
> > 
> > I think we probably should fix this specific case, and then add a check
> > to opr_sanity.sql or such.
> > 
> > As far as I can tell pg_config() was marked as such since its addition
> > in [2]. Joe, I assume this wasn't intentional?
> 
> Not intentional. Though, sitting here chatting with Stephen about it, I
> am now wondering if pg_config() should actually be marked immutable:
> 
> select * from pg_config() where name = 'VERSION';
>   name   | setting
> -+-
>  VERSION | PostgreSQL 10.5
> (1 row)
> 
> [...upgrade the postgres binaries...]
> 
> select * from pg_config() where name = 'VERSION';
>   name   | setting
> -+-
>  VERSION | PostgreSQL 10.6
> (1 row)
> 
> So the correct answer is probably to mark pg_config() stable, but it
> still seems to be parallel safe to me.

I don't think we should consider immutability to mean anything across
major versions. What'd be helped by doing that? We'd have to rule out
any behaviour change to any immutable function for that to make
sense. Including making an immutable function not immutable anymore.

- Andres

Re: IMMUTABLE and PARALLEL SAFE function markings

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-27 00:47:47 +0100, Vik Fearing wrote:
> > On 27/11/2018 00:39, Andres Freund wrote:
> > > On 2018-11-27 00:33:10 +0100, Vik Fearing wrote:
> > >> On 26/11/2018 22:23, Gajus Kuizinas wrote:
> > >>> I was wondering what is the reason IMMUTABLE functions are not by
> > >>> default PARALLEL SAFE and if the default behaviour could be changed to
> > >>> make IMMUTABLE functions PARALLEL SAFE?
> > >>
> > >> I think I have to concur with this.  When is an immutable function not
> > >> parallel safe?
> > >>
> > >> Sure it could be mislabeled as immutable but it could just as easily be
> > >> mislabeled as parallel safe.  And we already treat fake immutable
> > >> functions as user errors, for example in indexes.
> > > 
> > > I think it'd introduce more problems than it'd solve. Either you ignore
> > > the proparallel setting - resulting in broken catalog querying - or you
> > > have to have a decent amount of special behaviour that an explicit ALTER
> > > FUNCTION ... IMMUTABLE | STABLE | VOLATILE and SET PARALLEL { UNSAFE
> > > | RESTRICTED | SAFE } would also need to change the respective other
> > > category.
> > 
> > Surely a simple rule could be made that provolatile='i' trumps
> > proparallel.  No need to make them agree.
> > 
> > The default catalogs should agree (and I would expect the sanity checks
> > to look for that) but here we're talking about user functions.
> 
> I think it'd be entirely unacceptable that
>   SELECT * FROM pg_proc WHERE proparallel = 's'
> wouldn't actually return all the functions that are parallel safe.

Agreed, but I could see us having a regression test which complains if
it finds any which are marked as immutable but aren't parallel safe.

That said, I do *not* think we should make any assumptions here- users
incorrectly mark things all the time but we shouldn't encourage that and
we shouldn't assume that functions marked as immutable are parallel
safe.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: pg_config wrongly marked as not parallel safe?

[Joe Conway]

On 11/26/18 6:45 PM, Andres Freund wrote:
> Hi,
> 
> Triggered by the thread at [1] I looked for functions marked as
> immutable but not parallel safe.
> 
> postgres[19492][1]=# SELECT oid::regprocedure, provolatile, proparallel FROM 
> pg_proc WHERE provolatile = 'i' AND proparallel != 's';
> ┌─┬─┬─┐
> │ oid │ provolatile │ proparallel │
> ├─┼─┼─┤
> │ pg_config() │ i   │ r   │
> └─┴─┴─┘
> (1 row)
> 
> # pg_config
> { oid => '3400', descr => 'pg_config binary as a function',
>   proname => 'pg_config', prorows => '23', proretset => 't', proparallel => 
> 'r',
>   prorettype => 'record', proargtypes => '', proallargtypes => '{text,text}',
>   proargmodes => '{o,o}', proargnames => '{name,setting}',
>   prosrc => 'pg_config' },
> 
> so that function is marked as immutable but not parallel safe, without
> an explanation for that odd combination.
> 
> Now obviously I don't think it practially matters for pg_config(), but
> it seems unnecessarily confusing as a general matter.
> 
> I think we probably should fix this specific case, and then add a check
> to opr_sanity.sql or such.
> 
> As far as I can tell pg_config() was marked as such since its addition
> in [2]. Joe, I assume this wasn't intentional?

Not intentional. Though, sitting here chatting with Stephen about it, I
am now wondering if pg_config() should actually be marked immutable:

select * from pg_config() where name = 'VERSION';
  name   | setting
-+-
 VERSION | PostgreSQL 10.5
(1 row)

[...upgrade the postgres binaries...]

select * from pg_config() where name = 'VERSION';
  name   | setting
-+-
 VERSION | PostgreSQL 10.6
(1 row)

So the correct answer is probably to mark pg_config() stable, but it
still seems to be parallel safe to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

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

[Tomas Vondra]

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.


regards

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

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andres Freund]

On 2018-11-27 00:47:47 +0100, Vik Fearing wrote:
> On 27/11/2018 00:39, Andres Freund wrote:
> > Hi,
> > 
> > On 2018-11-27 00:33:10 +0100, Vik Fearing wrote:
> >> On 26/11/2018 22:23, Gajus Kuizinas wrote:
> >>> I was wondering what is the reason IMMUTABLE functions are not by
> >>> default PARALLEL SAFE and if the default behaviour could be changed to
> >>> make IMMUTABLE functions PARALLEL SAFE?
> >>
> >> I think I have to concur with this.  When is an immutable function not
> >> parallel safe?
> >>
> >> Sure it could be mislabeled as immutable but it could just as easily be
> >> mislabeled as parallel safe.  And we already treat fake immutable
> >> functions as user errors, for example in indexes.
> > 
> > I think it'd introduce more problems than it'd solve. Either you ignore
> > the proparallel setting - resulting in broken catalog querying - or you
> > have to have a decent amount of special behaviour that an explicit ALTER
> > FUNCTION ... IMMUTABLE | STABLE | VOLATILE and SET PARALLEL { UNSAFE
> > | RESTRICTED | SAFE } would also need to change the respective other
> > category.
> 
> Surely a simple rule could be made that provolatile='i' trumps
> proparallel.  No need to make them agree.
> 
> The default catalogs should agree (and I would expect the sanity checks
> to look for that) but here we're talking about user functions.

I think it'd be entirely unacceptable that
  SELECT * FROM pg_proc WHERE proparallel = 's'
wouldn't actually return all the functions that are parallel safe.
Greetings,

Andres Freund

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 27/11/2018 00:39, Andres Freund wrote:
> Hi,
> 
> On 2018-11-27 00:33:10 +0100, Vik Fearing wrote:
>> On 26/11/2018 22:23, Gajus Kuizinas wrote:
>>> I was wondering what is the reason IMMUTABLE functions are not by
>>> default PARALLEL SAFE and if the default behaviour could be changed to
>>> make IMMUTABLE functions PARALLEL SAFE?
>>
>> I think I have to concur with this.  When is an immutable function not
>> parallel safe?
>>
>> Sure it could be mislabeled as immutable but it could just as easily be
>> mislabeled as parallel safe.  And we already treat fake immutable
>> functions as user errors, for example in indexes.
> 
> I think it'd introduce more problems than it'd solve. Either you ignore
> the proparallel setting - resulting in broken catalog querying - or you
> have to have a decent amount of special behaviour that an explicit ALTER
> FUNCTION ... IMMUTABLE | STABLE | VOLATILE and SET PARALLEL { UNSAFE
> | RESTRICTED | SAFE } would also need to change the respective other
> category.

Surely a simple rule could be made that provolatile='i' trumps
proparallel.  No need to make them agree.

The default catalogs should agree (and I would expect the sanity checks
to look for that) but here we're talking about user functions.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

pg_config wrongly marked as not parallel safe?

[Andres Freund]

Hi,

Triggered by the thread at [1] I looked for functions marked as
immutable but not parallel safe.

postgres[19492][1]=# SELECT oid::regprocedure, provolatile, proparallel FROM 
pg_proc WHERE provolatile = 'i' AND proparallel != 's';
┌─┬─┬─┐
│ oid │ provolatile │ proparallel │
├─┼─┼─┤
│ pg_config() │ i   │ r   │
└─┴─┴─┘
(1 row)

# pg_config
{ oid => '3400', descr => 'pg_config binary as a function',
  proname => 'pg_config', prorows => '23', proretset => 't', proparallel => 'r',
  prorettype => 'record', proargtypes => '', proallargtypes => '{text,text}',
  proargmodes => '{o,o}', proargnames => '{name,setting}',
  prosrc => 'pg_config' },

so that function is marked as immutable but not parallel safe, without
an explanation for that odd combination.

Now obviously I don't think it practially matters for pg_config(), but
it seems unnecessarily confusing as a general matter.

I think we probably should fix this specific case, and then add a check
to opr_sanity.sql or such.

As far as I can tell pg_config() was marked as such since its addition
in [2]. Joe, I assume this wasn't intentional?

Greetings,

Andres Freund

[1] 
https://www.postgresql.org/message-id/CA+6d-n4dOakgLu2gsTfk9uD2CC9ueNCg+z_mnXA2-=qaod1...@mail.gmail.com
[2] 
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a5c43b886942e96ec5c745041f2d6a50c3205147

Re: IMMUTABLE and PARALLEL SAFE function markings

[Andres Freund]

Hi,

On 2018-11-27 00:33:10 +0100, Vik Fearing wrote:
> On 26/11/2018 22:23, Gajus Kuizinas wrote:
> > I was wondering what is the reason IMMUTABLE functions are not by
> > default PARALLEL SAFE and if the default behaviour could be changed to
> > make IMMUTABLE functions PARALLEL SAFE?
> 
> I think I have to concur with this.  When is an immutable function not
> parallel safe?
> 
> Sure it could be mislabeled as immutable but it could just as easily be
> mislabeled as parallel safe.  And we already treat fake immutable
> functions as user errors, for example in indexes.

I think it'd introduce more problems than it'd solve. Either you ignore
the proparallel setting - resulting in broken catalog querying - or you
have to have a decent amount of special behaviour that an explicit ALTER
FUNCTION ... IMMUTABLE | STABLE | VOLATILE and SET PARALLEL { UNSAFE
| RESTRICTED | SAFE } would also need to change the respective other
category.

Greetings,

Andres Freund

Re: IMMUTABLE and PARALLEL SAFE function markings

[Vik Fearing]

On 26/11/2018 22:23, Gajus Kuizinas wrote:
> I was wondering what is the reason IMMUTABLE functions are not by
> default PARALLEL SAFE and if the default behaviour could be changed to
> make IMMUTABLE functions PARALLEL SAFE?

I think I have to concur with this.  When is an immutable function not
parallel safe?

Sure it could be mislabeled as immutable but it could just as easily be
mislabeled as parallel safe.  And we already treat fake immutable
functions as user errors, for example in indexes.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Re: [RFC] Removing "magic" oids

[Andres Freund]

Hi,

On 2018-11-21 16:30:07 +0100, Andreas Karlsson wrote:
> On 11/21/18 1:07 AM, Andres Freund wrote:
> > Let's see what I broke :/
> 
> There is a small typo in the old release notes.

Thanks, pushed!

Greetings,

Andres Freund

Re: pg_stat_ssl additions

[Thomas Munro]

On Thu, Oct 18, 2018 at 11:05 AM Peter Eisentraut
 wrote:
> - Adds tests under src/test/ssl/ for the pg_stat_ssl view.

Hi Peter,

Your new tests fail when run by cfbot (Ubuntu), and also on my Debian
buster machine, but pass on my FreeBSD 13-CURRENT box.  I think the
problem is that you match cipher names with \w+, but some cipher names
sometimes (on some library versions?) have hyphens in them instead of
underscores.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/459620336

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

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

[Tomas Vondra]

On 11/26/18 11:29 PM, Thomas Munro wrote:
> On Mon, Sep 3, 2018 at 11:17 AM Tomas Vondra
>  wrote:
>> Attached is an updated version of the patch series, adopting a couple of
>> improvements - both for MCV lists and histograms.
> 
> Hello Tomas,
> 
> FYI, here are a couple of warnings from GCC (I just noticed because I
> turned on -Werror on cfbot so your patch turned red):
> 
> extended_stats.c: In function ‘statext_clauselist_selectivity’:
> extended_stats.c:1227:6: error: ‘other_sel’ may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
>   sel = mcv_sel + other_sel;
>   ^
> extended_stats.c:1091:5: note: ‘other_sel’ was declared here
>  other_sel,
>  ^
> extended_stats.c:1227:6: error: ‘mcv_sel’ may be used uninitialized in
> this function [-Werror=maybe-uninitialized]
>   sel = mcv_sel + other_sel;
>   ^
> extended_stats.c:1087:5: note: ‘mcv_sel’ was declared here
>  mcv_sel,
>  ^
> 

Thanks, I'll fix that in the next version of the patch I'm working on.


cheers

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

Re: [RFC] Removing "magic" oids

[Andres Freund]

Hi,

On 2018-11-22 17:12:31 -0500, Andrew Dunstan wrote:
> 
> On 11/22/18 4:14 PM, Andres Freund wrote:
> > Hi,
> > 
> > On 2018-11-21 23:32:07 -0500, Andrew Dunstan wrote:
> > > On 11/21/18 7:14 PM, Andres Freund wrote:
> > > >Could you check whether you
> > > > still encounter the issue after applying the attached fix?
> > > > 
> > > 
> > > This has largely fixed the problem, so I think this should be applied.
> > Cool, will do so tomorrow or such. Thanks for testing.

Sorry, the long weekend delayed this. Pushed now.


> > > 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.

Greetings,

Andres Freund

Re: Refactoring the checkpointer's fsync request queue

[Thomas Munro]

On Fri, Nov 23, 2018 at 5:45 PM Thomas Munro
 wrote:
> I do have a new plan though...

Ugh.  The plan in my previous email doesn't work, I was confused about
the timing of the buffer header update.  Back to the drawing board.

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

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

[Thomas Munro]

On Mon, Sep 3, 2018 at 11:17 AM Tomas Vondra
 wrote:
> Attached is an updated version of the patch series, adopting a couple of
> improvements - both for MCV lists and histograms.

Hello Tomas,

FYI, here are a couple of warnings from GCC (I just noticed because I
turned on -Werror on cfbot so your patch turned red):

extended_stats.c: In function ‘statext_clauselist_selectivity’:
extended_stats.c:1227:6: error: ‘other_sel’ may be used uninitialized
in this function [-Werror=maybe-uninitialized]
  sel = mcv_sel + other_sel;
  ^
extended_stats.c:1091:5: note: ‘other_sel’ was declared here
 other_sel,
 ^
extended_stats.c:1227:6: error: ‘mcv_sel’ may be used uninitialized in
this function [-Werror=maybe-uninitialized]
  sel = mcv_sel + other_sel;
  ^
extended_stats.c:1087:5: note: ‘mcv_sel’ was declared here
 mcv_sel,
 ^

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

Re: csv format for psql

[Pavel Stehule]

>
> Pushed with that correction and some other last-minute review.
>

Great! Thank you

Pavel


>
> regards, tom lane
>
>

IMMUTABLE and PARALLEL SAFE function markings

[Gajus Kuizinas]

I have defined a function "is_not_distinct" for the purpose of creating a
custom operator "===".

CREATE FUNCTION is_not_distinct(a anyelement, b anyelement)
returns boolean
language sql as $$
  select a is not distinct from b;
$$
IMMUTABLE;

CREATE OPERATOR === (
  LEFTARG = anyelement,
  RIGHTARG = anyelement,
  PROCEDURE = is_not_distinct,
  NEGATOR = !==
);

I have observed that the resulting queries were executed without using
parallelisation.

I have learned by asking on Freenode that the reason my queries are not
using parallelisation is because I have not configured PARALLEL SAFE.

I find it counter-intuitive that a function with IMMUTABLE marking would
require an explicit PARALLEL SAFE. It would seem logical that in all cases
where a function is appropriately market as IMMUTABLE it would also
be PARALLEL SAFE.

I was wondering what is the reason IMMUTABLE functions are not by
default PARALLEL SAFE and if the default behaviour could be changed to make
IMMUTABLE functions PARALLEL SAFE?

Thank you,

Gajus
ᐧ

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

[Stephen Frost]

Greetings,

* Sergei Kornilov (s...@zsrv.org) wrote:
> > I'd encourage you to look through the diff after you're finished hacking
> > before sending it to the list, in case things get left in that should be
> > removed, as below...
> I am so sorry. I have a look before sending, but...
> It's night in my timezone. I will fix tomorrow.

Sure, I don't think there's any need to rush any of this.

> >>  + if (targetSettingsCount > 1)
> >>  + ereport(FATAL,
> >>  + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >>  + errmsg("can not specify multiple recovery targets")));
> >
> > Doesn't look like you changed this based on my prior comment..?
> Do you mean this one?
> > I think I would have done 'if (targetSettingsCount != 1)' here.
> To be sure, we need check we have one recovery target without standby mode 
> and none or one in standby mode? Or some another check?

Right, I think that's what the idea was, although that might require
something for the explicit 'recover to the end case', now that I think
about it, so perhaps the >1 isn't so bad.  Still seems a bit odd to me
though and I do wonder if there might be a better approach.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Inadequate executor locking of indexes

[David Rowley]

On Mon, 26 Nov 2018 at 18:57, Amit Langote
 wrote:
> On 2018/11/26 14:25, David Rowley wrote:
> > I'm making efforts to delay per-partition work even further in the
> > executor, for example locking of the per-partition result relations
> > until after run-time pruning would be a significant win for
> > partitioned tables with many partitions when generic plans are in use.
> > Moving things back to InitPlan() flies in the face of that work.
> >
> > [1] https://commitfest.postgresql.org/20/1778/
>
> That's an interesting point.  Although, considering the concerns that Tom
> raised about the same index relation being locked such that lock-strength
> upgrade occurs (his example containing two CTEs), we'll have to find a way
> to do the ModifyTable run-time pruning such that result relations and
> their indexes (possibly under multiple ModifyTable nodes) are locked with
> RowExclusiveLock before they're locked with AccessShareLock lock as scan
> relations.  For example, we might be able to find a way to do the
> ModifyTable run-time pruning in InitPlan before initializing plan trees.

I thought my idea of the processing the rangetable at the end of
planning to determine the strongest lock per relation would have
solved that.

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

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

[Sergei Kornilov]

Hi

> I'd encourage you to look through the diff after you're finished hacking
> before sending it to the list, in case things get left in that should be
> removed, as below...
I am so sorry. I have a look before sending, but...
It's night in my timezone. I will fix tomorrow.

>>  + if (targetSettingsCount > 1)
>>  + ereport(FATAL,
>>  + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>>  + errmsg("can not specify multiple recovery targets")));
>
> Doesn't look like you changed this based on my prior comment..?
Do you mean this one?
> I think I would have done 'if (targetSettingsCount != 1)' here.
To be sure, we need check we have one recovery target without standby mode and 
none or one in standby mode? Or some another check?

regards, Sergei

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

[Stephen Frost]

Greetings,

* Sergei Kornilov (s...@zsrv.org) wrote:
> Updated patch attached:
> - added testcase to verify database does not start with multiple recovery 
> targets
> - recovery_target = immediate was replaced with recovery_target_immediate 
> bool GUC

I'd encourage you to look through the diff after you're finished hacking
before sending it to the list, in case things get left in that should be
removed, as below...

> diff --git a/src/backend/access/transam/xlog.c 
> b/src/backend/access/transam/xlog.c
> index c80b14e..cd29606 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -261,13 +261,21 @@ static bool restoredFromArchive = false;
>  static char *replay_image_masked = NULL;
>  static char *master_image_masked = NULL;
>  
> +/* recovery_target* original GUC values */
> +char *recovery_target_string;

This shouldn't be here now, should it?

> +char *recovery_target_xid_string;
> +char *recovery_target_time_string;
> +char *recovery_target_name_string;
> +char *recovery_target_lsn_string;

>  /* options formerly taken from recovery.conf for archive recovery */

Shouldn't all of the above be listed under this comment..?

>  char*recoveryRestoreCommand = NULL;
>  char*recoveryEndCommand = NULL;
>  char*archiveCleanupCommand = NULL;
> -RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
> +static RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
>  bool recoveryTargetInclusive = true;
>  int  recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
> +bool recoveryTargetImmediate;

Seems like this should, at least, be close to the char*'s that are
defining the other ways to specify a recovery targer.

>  TransactionId recoveryTargetXid;
>  TimestampTz recoveryTargetTime;
>  char*recoveryTargetName;
> @@ -5381,9 +5389,42 @@ readRecoverySignalFile(void)
>  static void
>  validateRecoveryParameters(void)
>  {
> + uint8 targetSettingsCount = 0;
> +
>   if (!ArchiveRecoveryRequested)
>   return;
>  
> + /* Check for mutually exclusive target actions */
> + if (recoveryTargetImmediate)
> + {
> + ++targetSettingsCount;
> + recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
> + }
> + if (strcmp(recovery_target_name_string, "") != 0)
> + {
> + ++targetSettingsCount;
> + recoveryTarget = RECOVERY_TARGET_NAME;
> + }
> + if (strcmp(recovery_target_lsn_string, "") != 0)
> + {
> + ++targetSettingsCount;
> + recoveryTarget = RECOVERY_TARGET_LSN;
> + }
> + if (strcmp(recovery_target_xid_string, "") != 0)
> + {
> + ++targetSettingsCount;
> + recoveryTarget = RECOVERY_TARGET_XID;
> + }
> + if (strcmp(recovery_target_time_string, "") != 0)
> + {
> + ++targetSettingsCount;
> + recoveryTarget = RECOVERY_TARGET_TIME;
> + }
> + if (targetSettingsCount > 1)
> + ereport(FATAL,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +  errmsg("can not specify multiple recovery 
> targets")));

Doesn't look like you changed this based on my prior comment..?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: csv format for psql

[Tom Lane]

"Daniel Verite"  writes:
>> A proposed fix is attached. print_csv_vertical() is left unchanged
>> because it's not possible currently to end up with \. alone
>> on a line with the expanded display

> On second thought, it is possible
> postgres=# \pset format csv
> Output format is csv.
> postgres=# \pset fieldsep_csv '.'
> Field separator for CSV is ".".
> postgres=# \x
> Expanded display is on.
> postgres=# select '' as "\" ;
> \.

Ugh.  There's more to that than meets the eye; consider also

\pset csv_fieldsep '.'
select '\' as d1, '' as d2;

or the reverse case where we set the fieldsep to '\' and then
emit a second field containing '.'.

I think that the approach you're using doesn't really scale to handle
all these cases sanely, so what I did instead was just to make
csv_print_field force field quoting if any of these cases could
possibly apply.  That will result in excess quoting in some
cases, but I think that's fine, since they're all pretty uncommon.

(BTW, it seems only chance that the server's CSV mode isn't also
subject to this case, but since it always quotes empty fields,
I think we're OK there.)

Pushed with that correction and some other last-minute review.

regards, tom lane

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

[Sergei Kornilov]

Hello

Updated patch attached:
- added testcase to verify database does not start with multiple recovery 
targets
- recovery_target = immediate was replaced with recovery_target_immediate bool 
GUC

thank you!

regards, Sergeidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index db1a2d4..49caa0c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3217,19 +3217,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
  
   By default, recovery will recover to the end of the WAL log. The
   following parameters can be used to specify an earlier stopping point.
-  At most one of recovery_target,
+  At most one of recovery_target_immediate,
   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 fatal error will be raised.
   These parameters can only be set at server start.
  
 
  
- 
-  recovery_target = 'immediate'
+ 
+  recovery_target_immediate (boolean)
   
-recovery_target configuration parameter
+recovery_target_immediate configuration parameter
   
   
   
@@ -3239,10 +3239,6 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 from an online backup, this means the point where taking the backup
 ended.

-   
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
-   
   
  
 
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c80b14e..cd29606 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -261,13 +261,21 @@ static bool restoredFromArchive = false;
 static char *replay_image_masked = NULL;
 static char *master_image_masked = NULL;
 
+/* recovery_target* original GUC values */
+char *recovery_target_string;
+char *recovery_target_xid_string;
+char *recovery_target_time_string;
+char *recovery_target_name_string;
+char *recovery_target_lsn_string;
+
 /* options formerly taken from recovery.conf for archive recovery */
 char	   *recoveryRestoreCommand = NULL;
 char	   *recoveryEndCommand = NULL;
 char	   *archiveCleanupCommand = NULL;
-RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
+static RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
 bool		recoveryTargetInclusive = true;
 int			recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
+bool 		recoveryTargetImmediate;
 TransactionId recoveryTargetXid;
 TimestampTz recoveryTargetTime;
 char	   *recoveryTargetName;
@@ -5381,9 +5389,42 @@ readRecoverySignalFile(void)
 static void
 validateRecoveryParameters(void)
 {
+	uint8 targetSettingsCount = 0;
+
 	if (!ArchiveRecoveryRequested)
 		return;
 
+	/* Check for mutually exclusive target actions */
+	if (recoveryTargetImmediate)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
+	}
+	if (strcmp(recovery_target_name_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_NAME;
+	}
+	if (strcmp(recovery_target_lsn_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_LSN;
+	}
+	if (strcmp(recovery_target_xid_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_XID;
+	}
+	if (strcmp(recovery_target_time_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_TIME;
+	}
+	if (targetSettingsCount > 1)
+		ereport(FATAL,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("can not specify multiple recovery targets")));
+
 	/*
 	 * Check for compulsory parameters
 	 */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6497393..9d6879c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -198,8 +198,6 @@ static const char *show_log_file_mode(void);
 static const char *show_data_directory_mode(void);
 static bool check_recovery_target_timeline(char **newval, void **extra, GucSource source);
 static void assign_recovery_target_timeline(const char *newval, void *extra);
-static bool check_recovery_target(char **newval, void **extra, GucSource source);
-static void assign_recovery_target(const char *newval, void *extra);
 static bool check_recovery_target_xid(char **newval, void **extra, GucSource source);
 static void assign_recovery_target_xid(const char *newval, void *extra);
 static bool check_recovery_target_time(char **newval, void **extra, GucSource source);
@@ -549,12 +547,6 @@ static bool data_checksums;
 static bool integer_datetimes;
 static bool assert_enabled;
 static char *recovery_target_timeline_string;
-static char *recovery_target_string;
-static char *recovery_target_xid_string;
-static char *recovery_target_time_string;
-static char *recovery_target_name_string;
-static

Re: Make foo=null a warning by default.

[Dmitry Dolgov]

> On Tue, Aug 7, 2018 at 11:19 PM Bruce Momjian  wrote:
>
> On Mon, Jul 16, 2018 at 11:37:28AM -0400, Tom Lane wrote:
> > Heikki Linnakangas  writes:
> > > On 16/07/18 18:10, Tom Lane wrote:
> > >> TBH I'm not really excited about investing any work in this area at all.
> > >> Considering how seldom we hear any questions about transform_null_equals
> > >> anymore[1], I'm wondering if we couldn't just rip the "feature" out
> > >> entirely.
> >
> > > Yeah, I was wondering about that too. But Fabien brought up a completely
> > > new use-case for this: people learning SQL. For beginners who don't
> > > understand the behavior of NULLs yet, I can see a warning or error being
> > > useful training wheels. Perhaps a completely new "training_wheels=on"
> > > option, which could check may for many other beginner errors, too, would
> > > be better for that.
> >
> > Agreed --- but what we'd want that to do seems only vaguely related to
> > the existing behavior of transform_null_equals.  As an example, we
> > intentionally made transform_null_equals *not* trigger on
> >
> >   CASE x WHEN NULL THEN ...
> >
> > but a training-wheels warning for that would likely be reasonable.
> >
> > For that matter, many of the old threads about this are complaining
> > about nulls that aren't simple literals in the first place.  I wonder
> > whether a training-wheels feature that whined *at runtime* about null
> > WHERE-qual or case-test results would be more useful than a parser
> > check.
>
> I will again say I would love to see this as part of a wholesale
> "novice" mode which warns of generally bad SQL practices.  I don't see
> this one item alone as sufficiently useful.

Valid point. Maybe then at least we can outline what kind of bad SQL practices
could be included into this "novice mode" to make it sufficiently useful?
Otherwise this sounds like a boundless problem.

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

[David Steele]

On 11/26/18 1:15 PM, Andres Freund wrote:
> On 2018-11-26 15:09:43 -0300, Alvaro Herrera wrote:
>> On 2018-Nov-26, Stephen Frost wrote:
>>
>>> I would think we'd have the different GUCs and then the check functions
>>> would only validate that they're valid inputs and then when we get to
>>> the point where we're starting to do recovery we check and make sure
>>> we've been given a sane overall configuration- which means that only
>>> *one* is set, and it matches the recovery target requested.
>>
>> I don't quite understand why it isn't sensible to specify more than one
>> and just stop recovery (or whatever) when at least one of them becomes
>> true.  Maybe I want to terminate just before commit of transaction
>> 12345, but no later than 2018-11-11 12:47 in any case.
> 
> +1

-1. At least for now.

Prior to this patch the last target specified in recovery.conf was the
one used, not a combination of them.

The committed patch did not propose to change that behavior as far as I
can see.  Since there is no way to determine the order of GUCs like
there was for options in recovery.conf, I think it makes sense to
restrict it to a single target type for now.  This is not exactly the
behavior we had before but I think it comes the closest.

Allowing multiple targets could be considered as a separate patch if
anyone is interested.

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

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

[Stephen Frost]

Greetings,

* Sergei Kornilov (s...@zsrv.org) wrote:
> > I think I would have done 'if (targetSettingsCount != 1)' here.
> Streaming replication does not have any recovery_target.
> Well, i can check StandbyModeRequested too and allow 0 recovery_target only 
> for standby mode.

Ah.  Agreed.

> >>  -# Multiple targets
> >>  -# Last entry has priority (note that an array respects the order of items
> >>  -# not hashes).
> >
> > You could (and imv should) include a test that throws an expected error
> > if multiple targets are specified.
> We have some example for checking unable-to-start DB? Did not find yet

The TAP system in general supports expected failures, but that might
only be with the simple binaries, not sure.  I know src/bin/pg_dump has
those though.

> > Let’s at least fix the currently committed patch before adding new features 
> > or changing how things in recovery work.
> +1

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: csv format for psql

[Daniel Verite]

I wrote:

> A proposed fix is attached. print_csv_vertical() is left unchanged
> because it's not possible currently to end up with \. alone
> on a line with the expanded display

On second thought, it is possible

postgres=# \pset format csv
Output format is csv.

postgres=# \pset fieldsep_csv '.'
Field separator for CSV is ".".

postgres=# \x
Expanded display is on.

postgres=# select '' as "\" ;
\.

PFA an upgraded fix.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 3b07280..d9c701f 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -1793,7 +1793,16 @@ print_csv_text(const printTableContent *cont, FILE *fout)
if (cont->opt->start_table && !cont->opt->tuples_only)
{
/* print headers */
-   for (ptr = cont->headers; *ptr; ptr++)
+
+   /*
+* A \. alone on a line must be quoted to be compatible with 
COPY CSV,
+* for which \. is an end-of-data marker.
+*/
+   if (cont->ncolumns == 1 && strcmp(*cont->headers, "\\.") == 0)
+   {
+   csv_escaped_print(*cont->headers, fout);
+   }
+   else for (ptr = cont->headers; *ptr; ptr++)
{
if (ptr != cont->headers)
fputs(cont->opt->fieldSepCsv, fout);
@@ -1805,7 +1814,14 @@ print_csv_text(const printTableContent *cont, FILE *fout)
/* print cells */
for (i = 0, ptr = cont->cells; *ptr; i++, ptr++)
{
-   csv_print_field(*ptr, fout, cont->opt->fieldSepCsv);
+   /*
+* A \. alone on a line must be quoted to be compatible with 
COPY CSV,
+* for which \. is an end-of-data marker.
+*/
+   if (cont->ncolumns == 1 && strcmp(*ptr, "\\.") == 0)
+   csv_escaped_print(*ptr, fout);
+   else
+   csv_print_field(*ptr, fout, cont->opt->fieldSepCsv);
if ((i + 1) % cont->ncolumns)
fputs(cont->opt->fieldSepCsv, fout);
else
@@ -1825,16 +1841,28 @@ print_csv_vertical(const printTableContent *cont, FILE 
*fout)
if (cancel_pressed)
return;
 
-   /* print name of column */
-   csv_print_field(cont->headers[i % cont->ncolumns], fout,
-   cont->opt->fieldSepCsv);
-
-   /* print field separator */
-   fputs(cont->opt->fieldSepCsv, fout);
+   /*
+* A \. alone on a line must be quoted to be compatible with 
COPY CSV,
+* for which \. is an end-of-data marker.
+*/
+   if (strcmp(cont->headers[i % cont->ncolumns], "\\") == 0
+   && strcmp(cont->opt->fieldSepCsv, ".") == 0
+   && **ptr == '\0')
+   {
+   csv_escaped_print("\\.", fout);
+   }
+   else
+   {
+   /* print name of column */
+   csv_print_field(cont->headers[i % cont->ncolumns], fout,
+   cont->opt->fieldSepCsv);
 
-   /* print field value */
-   csv_print_field(*ptr, fout, cont->opt->fieldSepCsv);
+   /* print field separator */
+   fputs(cont->opt->fieldSepCsv, fout);
 
+   /* print field value */
+   csv_print_field(*ptr, fout, cont->opt->fieldSepCsv);
+   }
fputc('\n', fout);
}
 }

Re: Copy function for logical replication slots

[Petr Jelinek]

Hi,

On 26/11/2018 01:29, Masahiko Sawada wrote:
> On Sun, Nov 25, 2018 at 12:27 AM Petr Jelinek
>  wrote:
>>
>> The more serious thing is:
>>
>>> + if (MyReplicationSlot)
>>> + ReplicationSlotRelease();
>>> +
>>> + /* Release the saved slot if exist while preventing double releasing 
>>> */
>>> + if (savedslot && savedslot != MyReplicationSlot)
>>
>> This won't work as intended as the ReplicationSlotRelease() will set
>> MyReplicationSlot to NULL, you might need to set aside MyReplicationSlot
>> to yet another temp variable inside this function prior to releasing it.
>>
> 
> You're right. I've fixed it by checking if we need to release the
> saved slot before releasing the origin slot. Is that right?
> Attached an updated patch.
> 

Sounds good.

I do have one more minor gripe after reading though again:

> +
> +   /*
> +* The requested wal lsn is no longer available. We don't 
> want to retry
> +* it, so raise an error.
> +*/
> +   if (!XLogRecPtrIsInvalid(requested_lsn))
> +   {
> +   char filename[MAXFNAMELEN];
> +
> +   XLogFileName(filename, ThisTimeLineID, segno, 
> wal_segment_size);
> +   ereport(ERROR,
> +   (errmsg("could not reserve WAL 
> segment %s", filename)));
> +   }

I would reword the comment to something like "The caller has requested a
specific wal lsn which we failed to reserve. We can't retry here as the
requested wal is no longer available." (It took me a while to understand
this part).

Also the ereport should have errcode as it's going to be thrown to user
sessions and it might be better if the error itself used same wording as
CheckXLogRemoved() and XLogRead() for consistency. What do you think?

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

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

[Sergei Kornilov]

Hello

> I think I would have done 'if (targetSettingsCount != 1)' here.
Streaming replication does not have any recovery_target.
Well, i can check StandbyModeRequested too and allow 0 recovery_target only for 
standby mode.

>>  -# Multiple targets
>>  -# Last entry has priority (note that an array respects the order of items
>>  -# not hashes).
>
> You could (and imv should) include a test that throws an expected error
> if multiple targets are specified.
We have some example for checking unable-to-start DB? Did not find yet

> Let’s at least fix the currently committed patch before adding new features 
> or changing how things in recovery work.
+1

regards, Sergei

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

[Andres Freund]

On 2018-11-26 13:30:18 -0500, Stephen Frost wrote:
> Well, we could start with: that isn’t how things work today, nor how it
> used to work before this patch, and we’ve not had anyone asking for it
> except for people on this thread making things up.

Thanks for assuming good faith.

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

[Stephen Frost]

Greetings,

On Mon, Nov 26, 2018 at 13:15 Andres Freund  wrote:

> On 2018-11-26 15:09:43 -0300, Alvaro Herrera wrote:
> > On 2018-Nov-26, Stephen Frost wrote:
> >
> > > I would think we'd have the different GUCs and then the check functions
> > > would only validate that they're valid inputs and then when we get to
> > > the point where we're starting to do recovery we check and make sure
> > > we've been given a sane overall configuration- which means that only
> > > *one* is set, and it matches the recovery target requested.
> >
> > I don't quite understand why it isn't sensible to specify more than one
> > and just stop recovery (or whatever) when at least one of them becomes
> > true.  Maybe I want to terminate just before commit of transaction
> > 12345, but no later than 2018-11-11 12:47 in any case.


I really have doubts about that being a serious use-case.  If you know the
xid then you almost certainly want everything before that xid, and you use
the time stamp when you don’t know the xid (which is pretty much always..).

+1


Well, we could start with: that isn’t how things work today, nor how it
used to work before this patch, and we’ve not had anyone asking for it
except for people on this thread making things up.

Let’s at least fix the currently committed patch before adding new features
or changing how things in recovery work.

Thanks!

Stephen

>

Re: allow online change primary_conninfo

[Sergei Kornilov]

Hi

>>  Hmm... I considered SIGHUP processing was in fast loop and therefore 
>> shutdown should be fast. But i recheck code and found a possible long loop 
>> without processing SIGHUP (in case we receive new data faster than writes to 
>> disk). Ok, i will revert back.
>>  How about write to WalRcvData only clobbered conninfo?
>
> I'm not sure I understand what you mean?
I am about my initial proposal with remove conninfo wrom WalRcvData - 
walreceiver may run some time with old conninfo and 
> without this information that seems hard to debug.
Earlier i thought walreceiver will shutdown fast on SIGHUP.

> The way I'd solve this is that
> that only walreceiver, at startup, writes out its conninfo/slot_name,
> sourcing the values from the GUCs. That way there's no issue with values
> being overwritten early.
In second patch i follow exactly this logic.

regards, Sergei

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

[Andres Freund]

On 2018-11-26 15:09:43 -0300, Alvaro Herrera wrote:
> On 2018-Nov-26, Stephen Frost wrote:
> 
> > I would think we'd have the different GUCs and then the check functions
> > would only validate that they're valid inputs and then when we get to
> > the point where we're starting to do recovery we check and make sure
> > we've been given a sane overall configuration- which means that only
> > *one* is set, and it matches the recovery target requested.
> 
> I don't quite understand why it isn't sensible to specify more than one
> and just stop recovery (or whatever) when at least one of them becomes
> true.  Maybe I want to terminate just before commit of transaction
> 12345, but no later than 2018-11-11 12:47 in any case.

+1

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

[Alvaro Herrera]

On 2018-Nov-26, Stephen Frost wrote:

> I would think we'd have the different GUCs and then the check functions
> would only validate that they're valid inputs and then when we get to
> the point where we're starting to do recovery we check and make sure
> we've been given a sane overall configuration- which means that only
> *one* is set, and it matches the recovery target requested.

I don't quite understand why it isn't sensible to specify more than one
and just stop recovery (or whatever) when at least one of them becomes
true.  Maybe I want to terminate just before commit of transaction
12345, but no later than 2018-11-11 12:47 in any case.

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

Re: csv format for psql

[Daniel Verite]

Tom Lane wrote:

> OK, reasonable arguments were made why not to allow multi-character
> separators.  Should we then match the server and insist on a single-byte
> separator?  It's a bit inconsistent if psql can be made to emit "csv"
> files that COPY can't read, especially when it's otherwise a subset
> of what COPY allows.

I seem to be the only one advocating to accept an Unicode
character here, and I won't insist on it.

There's still a minor annoyance to solve if we want to claim full
compatibility with COPY CSV: backslash followed by dot must be
quoted to avoid being interpreted as an end of data indicator.

A proposed fix is attached. print_csv_vertical() is left unchanged
because it's not possible currently to end up with \. alone
on a line with the expanded display: we'd need to allow
first for an empty field separator, I believe.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 3b07280..d31b6af 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -1793,7 +1793,16 @@ print_csv_text(const printTableContent *cont, FILE *fout)
if (cont->opt->start_table && !cont->opt->tuples_only)
{
/* print headers */
-   for (ptr = cont->headers; *ptr; ptr++)
+
+   /*
+* A \. alone on a line must be quoted to be compatible with 
COPY CSV,
+* for which \. is an end-of-data marker.
+*/
+   if (cont->ncolumns == 1 && strcmp(*cont->headers, "\\.") == 0)
+   {
+   csv_escaped_print(*cont->headers, fout);
+   }
+   else for (ptr = cont->headers; *ptr; ptr++)
{
if (ptr != cont->headers)
fputs(cont->opt->fieldSepCsv, fout);
@@ -1805,7 +1814,14 @@ print_csv_text(const printTableContent *cont, FILE *fout)
/* print cells */
for (i = 0, ptr = cont->cells; *ptr; i++, ptr++)
{
-   csv_print_field(*ptr, fout, cont->opt->fieldSepCsv);
+   /*
+* A \. alone on a line must be quoted to be compatible with 
COPY CSV,
+* for which \. is an end-of-data marker.
+*/
+   if (cont->ncolumns == 1 && strcmp(*ptr, "\\.") == 0)
+   csv_escaped_print(*ptr, fout);
+   else
+   csv_print_field(*ptr, fout, cont->opt->fieldSepCsv);
if ((i + 1) % cont->ncolumns)
fputs(cont->opt->fieldSepCsv, fout);
else

Re: allow online change primary_conninfo

[Andres Freund]

Hi,

On 2018-11-26 12:30:06 +0300, Sergei Kornilov wrote:
> Hmm... I considered SIGHUP processing was in fast loop and therefore shutdown 
> should be fast. But i recheck code and found a possible long loop without 
> processing SIGHUP (in case we receive new data faster than writes to disk). 
> Ok, i will revert back.
> How about write to WalRcvData only clobbered conninfo?

I'm not sure I understand what you mean?  The way I'd solve this is that
that only walreceiver, at startup, writes out its conninfo/slot_name,
sourcing the values from the GUCs. That way there's no issue with values
being overwritten early.

- Andres

Re: Handling of REGRESS_OPTS in MSVC for regression tests

[Andrew Dunstan]

On 11/26/18 12:43 AM, Michael Paquier wrote:

Hi all,

As mentioned on this thread, I have been fighting a bit with the
buildfarm when trying to introduce new PGXS options for isolation and
TAP tests:
https://www.postgresql.org/message-id/e1gr4d0-0002yj...@gemulon.postgresql.org

However it happens that we have more issues than the buildfarm failures
to begin with.  One of them it related to the way REGRESS_OPTS is
defined and handled across the whole tree for MSVC.

There are in the tree now a couple of paths using top_builddir or
top_srcdir:
$ git grep REGRESS_OPTS | grep top
contrib/dblink/Makefile:REGRESS_OPTS =
--dlpath=$(top_builddir)/src/test/regress
contrib/pg_stat_statements/Makefile:REGRESS_OPTS = --temp-config
$(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
src/test/modules/commit_ts/Makefile:REGRESS_OPTS =
--temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
src/test/modules/test_rls_hooks/Makefile:REGRESS_OPTS =
--temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf

Using top_builddir for dblink is for example fine as this needs to point
out to the place where builds of pg_regress are present.  However when
it comes to configuration files we should use top_builddir, and ought to
use top_srcdir instead as VPATH would complain about things gone
missing.  So the definition that we make out of it is correct.  However
there is an issue with MSVC which thinks that REGRESS_OPTS should only
include top_builddir.  This is actually fine per-se as all MSVC tests
run with make-installcheck, and not make-check, however I think that we
should allow top_srcdir to be switched on-the-fly as much as
top_builddir as top_srcdir could be used for something else.

Another way worse issue is that MSVC scripts ignore some configuration
values because of the way values of REGRESS_OPTS are parsed.  This
causes some sets of regression tests to never run on Windows.  For
example, here is what the command generated for pg_stat_statements, and
what happens with it:
c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/pg_regress/pg_regress \
   --bindir=c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/psql \
   --dbname=contrib_regression --temp-config pg_stat_statements
[...]
== running regression test queries==

=
  All 0 tests passed.
=

This causes the tests to pass, but to run nothing as test list is eaten
out as an option value for --temp-config.  In this case, what happens is
that --temp-config is set to "pg_stat_statements", leaving the test list
empty.  The same cannot happen with test_decoding as the buildfarm uses
a custom module to run its tests (still this module could go away with
PGXS options to control isolation tests).

Attached is a patch which makes MSVC scripts more intelligent by being
able to replace top_srcdir as well by topdir when fetching options.

Thanks to that, MSVC is able to run the tests correctly, by building a
proper command.  I think that this is a bug that should be back-patched,
because the tests just don't run, and we likely would miss regressions
because of that.

Unfortunately, if I were to commit that directly, the buildfarm would
turn immediately to red for all Windows members, because when checking
module and contrib tests an "installcheck" happens, and
shared_preload_libraries is not set in this case.  When working on the
switch to add isolation and TAP test support to PGXS, test_decoding has
been failing because the installed server did not have wal_level =
logical set up, which is one instance of that.  The buildfarm code
installing the Postgres instance on which the tests are run should
configure that properly I think, and one tweak that we could use for the
time being is to bypass tests of modules which cannot work yet, so as
the buildfarm keeps being green.  This way, this would not be a blocker
for the integration of the new PGXS infra for TAP and isolation.  And
those could be enabled back once the buildfarm code is able to set up a
proper configuration.  The following modules require some extra
configuration:
- commit_ts
- test_rls_hooks
- pg_stat_statements
- test_decoding (if its Makefile is rewritten)
- snapshot_too_old (if its Makefile is rewritten)

The buildfarm part requires Andrew Dunstan's input, and there is a bit
to sort out for the rest, so input from all is welcome.  Please note
that I would prefer if the tests which just cannot work yet are
disabled until the needed buildfarm infrastructure is needed.  Another
option could also be to switch contribcheck and modulescheck so as they
use a temporary installation, but that's a can of worms waiting to
explode as MSVC makes more complicated the search for initdb and such
(see the way upgradecheck happens for example), and this makes the tests
waaay longer to run.



It's not the buildfarm that is broken. Both contribcheck() and 
modulescheck() in vcregress.pl run in installcheck mode, 

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

[Stephen Frost]

Greetings,

* Sergei Kornilov (s...@zsrv.org) wrote:
> > I would think we'd have the different GUCs and then the check functions
> > would only validate that they're valid inputs and then when we get to
> > the point where we're starting to do recovery we check and make sure
> > we've been given a sane overall configuration- which means that only
> > *one* is set, and it matches the recovery target requested.
> How about something like attached patch?

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.

> + if (targetSettingsCount > 1)
> + ereport(FATAL,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +  errmsg("can not specify multiple recovery 
> targets")));

I think I would have done 'if (targetSettingsCount != 1)' here.

> -# Multiple targets
> -# Last entry has priority (note that an array respects the order of items
> -# not hashes).

You could (and imv should) include a test that throws an expected error
if multiple targets are specified.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Constraint documentation

[Lætitia Avrot]

Hi all,

Thank you for helping us to make that patch better.

It didn't
> seem worth blocking this patch for your proposed change (particularly
> since Lætitia seems to have given up on it already).
>
>
I haven't given up. As I said in the begining, Patrick and I were working
together on that patch. As he was doing well, I let him do the job and
focused on something else. The patch as it's now seems pretty good to me.
I agree working on F671 and F673 would be great. Sadly, I need to grow as a
developer to be able to do that.

Cheers,

Lætitia

-- 
*Think! Do you really need to print this email ? *
*There is no Planet B.*

RE: Shared Memory: How to use SYSV rather than MMAP ?

[REIX, Tony]

Hi Thomas,


You said:


I think we should respect the huge_pages GUC, as we do on Linux and
Windows (since there are downsides to using large pages, maybe not
everyone would want that).  It could even be useful to allow different
page sizes to be requested by GUC (I see that DB2 has an option to use
16GB pages -- yikes).  It also seems like a good idea to have a
shared_memory_type GUC as Andres proposed (see his link), instead of
using a compile time option.  I guess it was made a compile time
option because nobody could imagine wanting to go back to SysV shm!
(I'm still kinda surprised that MAP_ANONYMOUS memory can't be coaxed
into large pages by environment variables or loader controls, since
apparently other things like data segments etc apparently can, though
I can't find any text that says that's the case and I have no AIX
system).


I guess that you are talking about CPP & C variables:
  #ifndef MAP_HUGETLB
  HUGE_PAGES_ON
  HUGE_PAGES_TRY)
in addition to : huge_pages =  in postgresql.conf file.

For now, these variables for Huge Pages are used only for MMAP.
About SysV shared memory, as far as I know, shmget() options for AIX and Linux 
are different.
Moreover, AIX also provides Large Pages (16MB).

About Andres proposal, I've read his link. However, the patch he proposed:
0001-Add-shared_memory_type-GUC.patch
is no more available (Attachment not found).

I confirm that I got the SysV Shared Memory by means of a "compile time option".

About "still kinda surprised that MAP_ANONYMOUS memory can't be coaxed
into large pages by environment variables or loader controls" I confirm that,
on AIX, only 4K pages are available for mmap().

I do agree that options in the postgresql.conf file would be the best solution,
since the code for SysV shared memory and MMAP shared memory seems always 
present.

Regards,

Tony

Re: Covering GiST indexes

[Andrey Borodin]

Hi, Dmitry!

> 26 нояб. 2018 г., в 21:20, Dmitry Dolgov <9erthali...@gmail.com> написал(а):
> 
> Looks like this time the patch was picked up correctly, but there are some
> conflicts with the current master branch:
> http://cfbot.cputube.org/patch_20_1615.log
> Could you please rebase it one more time?

Here's rebased version. Thanks!

Best regards, Andrey Borodin.


0001-Covering-GiST-v5.patch
Description: Binary data

[PATCH] Tiny CREATE STATISTICS tab-completion cleanup

[Dagfinn Ilmari Mannsåker]

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.

- ilmari
-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen

>From dc4940b937f3e3d51fb76bfa970aac15be9476d2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Fri, 23 Nov 2018 15:21:22 +
Subject: [PATCH 1/2] Use wildcard to match parens after CREATE STATISTICS

Now that * can match in the middle of an alternative, we can use "(*)"
to match the whole parenthesised list, instead of checking manually
for the '(' and ')'.
---
 src/bin/psql/tab-complete.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 9dbd555166..90cc1fe215 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2390,9 +2390,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("(", "ON");
 	else if (Matches("CREATE", "STATISTICS", MatchAny, "("))
 		COMPLETE_WITH("ndistinct", "dependencies");
-	else if (HeadMatches("CREATE", "STATISTICS", MatchAny) &&
-			 previous_words[0][0] == '(' &&
-			 previous_words[0][strlen(previous_words[0]) - 1] == ')')
+	else if (Matches("CREATE", "STATISTICS", MatchAny, "(*)"))
 		COMPLETE_WITH("ON");
 	else if (HeadMatches("CREATE", "STATISTICS", MatchAny) &&
 			 TailMatches("FROM"))
-- 
2.19.2

Re: B-tree cache prefetches

[Andrey Borodin]

Hi, Thomas!

> 31 авг. 2018 г., в 2:40, Thomas Munro  
> написал(а):
> 
> [1] https://arxiv.org/pdf/1509.05053.pdf

I've implemented all of the strategies used in that paper.
On a B-tree page we have a line pointers ordered in key order and tuples 
residing at the end of the page. Tuples at the end are mostly "appended", i.e. 
they usually are accommodated at the end of the free space.
The ides of the attached patch is to try to order tuples in different 
strategies. This ordering happens during VACUUM and can be easily broken with 
single insert.
Strategies are switched by #define:
1. USE_SEQ_ORDER - just order tuples in order of line pointers. This strategy 
has no idea behind and is expected to work similar to baseline (no changes to 
code at all)
2. USE_EYZINGER_ORDER - depth-first topological search. If you always choose 
lower branch - your search is just a sequential read of bytes.
3. USE_VEB_ORDER - Van Emde Boas layout - try to pack 3 tuples (mid and two 
sub-mids) together. The key ideas is that you cache-miss when read mid, but 
tuples of both next steps are already fetched by that cache miss.
4. USE_BT_ORDER - order two sub-mids together so that you can prefetch both two 
possible next mids in a single cache prefetch.

Cache prefetches of B-tree binsearch are controlled by USE_PREFETCH.

I've tested all these strategies on my laptop (2.2GHz Intel i7, 16GB RAM, MacOS 
10.14.1)
./pgbench -i -s 25 postgres && ./pgbench -T 100 postgres
No changes in configs.

Here are results in tps:
without prefetch
baseline 1448.331199
seq  1433.737204
bt   1463.701527
veb  1457.586089
eyz  1483.654765

with prefetch
baseline 1486.585895
seq  1471.757042
bt   1480.169967
veb  1464.834678
eyz  1460.323392

This numbers are not statistically cleared, just one run was done for every 
number, experiment needs to be tuned anyway.
From this I can conclude:
1. It is hard to observe significant performance influence on pgbench. Maybe I 
should use some more representative B-tree performance tests?
2. Cache prefetches seem to increase performance without any layout strategies. 
But the difference is hair-splitting 2.6%
3. For implemented strategies my preliminary results corresponds to the result 
in a paper: Eyzinger layout is the best.
4. Among layout strategies with prefetch, bt-order strategy seems to be winner.


How can I improve experimental evaluation of these layout strategies?
Other thoughts? I'd be happy to hear any comment on this.


Best regards, Andrey Borodin.


0001-Implement-different-B-tree-page-layouts.patch
Description: Binary data

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

[Sergei Kornilov]

Hello

> I would think we'd have the different GUCs and then the check functions
> would only validate that they're valid inputs and then when we get to
> the point where we're starting to do recovery we check and make sure
> we've been given a sane overall configuration- which means that only
> *one* is set, and it matches the recovery target requested.
How about something like attached patch?

regards, Sergeidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index db1a2d4..d95d0a7 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 fatal error will be raised.
   These parameters can only be set at server start.
  
 
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c80b14e..01fbab8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -261,11 +261,18 @@ static bool restoredFromArchive = false;
 static char *replay_image_masked = NULL;
 static char *master_image_masked = NULL;
 
+/* recovery_target* original GUC values */
+char *recovery_target_string;
+char *recovery_target_xid_string;
+char *recovery_target_time_string;
+char *recovery_target_name_string;
+char *recovery_target_lsn_string;
+
 /* options formerly taken from recovery.conf for archive recovery */
 char	   *recoveryRestoreCommand = NULL;
 char	   *recoveryEndCommand = NULL;
 char	   *archiveCleanupCommand = NULL;
-RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
+static RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
 bool		recoveryTargetInclusive = true;
 int			recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
 TransactionId recoveryTargetXid;
@@ -5381,9 +5388,42 @@ readRecoverySignalFile(void)
 static void
 validateRecoveryParameters(void)
 {
+	uint8 targetSettingsCount = 0;
+
 	if (!ArchiveRecoveryRequested)
 		return;
 
+	/* Check for mutually exclusive target actions */
+	if (strcmp(recovery_target_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
+	}
+	if (strcmp(recovery_target_name_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_NAME;
+	}
+	if (strcmp(recovery_target_lsn_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_LSN;
+	}
+	if (strcmp(recovery_target_xid_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_XID;
+	}
+	if (strcmp(recovery_target_time_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_TIME;
+	}
+	if (targetSettingsCount > 1)
+		ereport(FATAL,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("can not specify multiple recovery targets")));
+
 	/*
 	 * Check for compulsory parameters
 	 */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6497393..fd85484 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -199,7 +199,6 @@ static const char *show_data_directory_mode(void);
 static bool check_recovery_target_timeline(char **newval, void **extra, GucSource source);
 static void assign_recovery_target_timeline(const char *newval, void *extra);
 static bool check_recovery_target(char **newval, void **extra, GucSource source);
-static void assign_recovery_target(const char *newval, void *extra);
 static bool check_recovery_target_xid(char **newval, void **extra, GucSource source);
 static void assign_recovery_target_xid(const char *newval, void *extra);
 static bool check_recovery_target_time(char **newval, void **extra, GucSource source);
@@ -549,12 +548,6 @@ static bool data_checksums;
 static bool integer_datetimes;
 static bool assert_enabled;
 static char *recovery_target_timeline_string;
-static char *recovery_target_string;
-static char *recovery_target_xid_string;
-static char *recovery_target_time_string;
-static char *recovery_target_name_string;
-static char *recovery_target_lsn_string;
-
 
 /* should be static, but commands/variable.c needs to get at this */
 char	   *role_string;
@@ -3385,7 +3378,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&recovery_target_string,
 		"",
-		check_recovery_target, assign_recovery_target, NULL
+		check_recovery_target, NULL, NULL
 	},
 	{
 		{"recovery_target_xid", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
@@ -11062,18 +11055,6 @@ check_recovery_target(char **newval, void **extra, GucSource source)
 	return true;
 }
 
-static void
-assign_recovery_target(const char *newval, void *extra)
-{
-	if (newval && strcmp(newval, "") != 0)
-		recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
-	else
-		/*
-		 * Reset recoveryTarget to RECOVERY_TARGET_UNSET to proper handl

[PATCH] Improve tab completion for CREATE TABLE

[Dagfinn Ilmari Mannsåker]

Hi hackers,

Please find attached a patch that adds the following tab completions for
CREATE TABLE:

- ( or PARTITION OF after the name
- options after the column list
- ON COMMIT actions for temp tables

Regards,

ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law

>From ed15e53b370d8ee4320961f17ed66cbf60621d28 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Fri, 23 Nov 2018 15:23:21 +
Subject: [PATCH] Tab complete more options for CREATE TABLE

- ( or PARTITION OF after the name
- options after the column list
- ON COMMIT actions for temp tables
---
 src/bin/psql/tab-complete.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 90cc1fe215..61b89c9370 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2412,6 +2412,19 @@ psql_completion(const char *text, int start, int end)
 	/* Limited completion support for partition bound specification */
 	else if (TailMatches("PARTITION", "OF", MatchAny))
 		COMPLETE_WITH("FOR VALUES", "DEFAULT");
+	/* Complete after CREATE TABLE  */
+	else if (TailMatches("CREATE", "TABLE", MatchAny) ||
+			 TailMatches("CREATE", "TEMP|TEMPORARY|UNLOGGED", "TABLE", MatchAny))
+		COMPLETE_WITH("(", "PARTITION OF");
+	/* Complete options after CREATE TABLE name (...) */
+	else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)") ||
+			 TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
+		COMPLETE_WITH("INHERITS (", "PARTITION BY", "WITH (", "TABLESPACE");
+	else if (TailMatches("CREATE", "TEMP|TEMPORARY", "TABLE", MatchAny, "(*)"))
+		COMPLETE_WITH("INHERITS (", "PARTITION BY", "WITH (", "ON COMMIT", "TABLESPACE");
+	/* Complete ON COMMIT actions for temp tables */
+	else if (TailMatches("ON", "COMMIT"))
+		COMPLETE_WITH("PRESERVE ROWS", "DELETE ROWS", "DROP");
 
 /* CREATE TABLESPACE */
 	else if (Matches("CREATE", "TABLESPACE", MatchAny))
-- 
2.19.2

Re: Updated backup APIs for non-exclusive backups

[Stephen Frost]

Greetings,

* Laurenz Albe (laurenz.a...@cybertec.at) wrote:
> On Sun, 2018-11-25 at 16:04 -0500, Stephen Frost wrote:
> > > Yes, you can come up with a post-backup script that somehow communicates
> > > with your pre-backup script to get the information, but it sure is
> > > inconvenient.  Simplicity is good in backup solutions, because complicated
> > > things tend to break more easily.
> > 
> > Not sure what communication is necessary here..?   The data needed for the 
> > backup
> > label file comes from pg_stop_backup in a non-exclusive backup.
> 
> Right, and pg_stop_backup has to be run from the "pre-backup" script.

No, pg_stop_backup has has to be run by a process that's been connected
since the pre-backup script ran, but you could certainly have some
process that starts from pre-backup, detaches, and then waits for a
signal from the post-backup script to run pg_stop_backup and record the
results somewhere.

Yes, that's rather annoying to do from a bash script, but I'm well past
the point of thinking that bash scripts are a good way to run backups in
PG.  Your efforts in this area might be better put to use by writing a
good tool which works with the non-exclusive API but can be called from
the pre/post backup scripts your backup solution provides.
Unfortunately, it seems pretty unlikely that there's a good generic way
to write such a tool or we could include it in PG.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Covering GiST indexes

[Dmitry Dolgov]

> On Mon, Jul 30, 2018 at 7:50 AM Andrey Borodin  wrote:
>
> Thanks, Thomas!
>
> > 30 июля 2018 г., в 3:58, Thomas Munro  
> > написал(а):
> >
> > On Sun, Jul 29, 2018 at 10:50 PM, Andrey Borodin  
> > wrote:
> >> Thanks! The problem appeared with commit 701fd0b [0] which dropped
> >> validation rules checked in failed test.  Here's the patch with fixed 
> >> tests.
> >
> > Thanks.  I received the attachment.
> >
> > Just as an FYI, something about the way your mail client encoded it
> > prevented the mail archives from picking it up
>
> I'll try to investigate this.
>
> Here's patch one more: another attempt to put it into archives.

Hi,

Looks like this time the patch was picked up correctly, but there are some
conflicts with the current master branch:
http://cfbot.cputube.org/patch_20_1615.log
Could you please rebase it one more time?

Re: csv format for psql

[Tom Lane]

"David G. Johnston"  writes:
> On Sun, Nov 25, 2018 at 6:27 PM Tom Lane  wrote:
>> 1. Are we limiting the separator to be a single-byte character or not?

> I agree with what others have said that expanding functionality in
> this direction is more likely to mask errors than be useful.

OK, reasonable arguments were made why not to allow multi-character
separators.  Should we then match the server and insist on a single-byte
separator?  It's a bit inconsistent if psql can be made to emit "csv"
files that COPY can't read, especially when it's otherwise a subset
of what COPY allows.

>> 2. Speaking of the field separator, I'm pretty desperately unhappy
>> with the choice of "fieldsep_csv" as the parameter name.[...]
>> We could avoid this self-inflicted confusion by choosing a different
>> parameter name.  I'd be good with "csv_fieldsep" or "csvfieldsep".

> Make sense to me - with the underscore personally.

Barring complaints, I'll switch it to "csv_fieldsep".

regards, tom lane