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

[Stas Kelvich]

> On 2 Mar 2017, at 01:20, Petr Jelinek  wrote:
> 
> The info gets removed on COMMIT PREPARED but at that point
> there is no real difference between replicating it as 2pc or 1pc since
> the 2pc behavior is for all intents and purposes lost at that point.
> 

If we are doing 2pc and COMMIT PREPARED happens then we should
replicate that without transaction body to the receiving servers since tx
is already prepared on them with some GID. So we need a way to construct
that GID.

It seems that last ~10 messages I’m failing to explain some points about this
topic. Or, maybe, I’m failing to understand some points. Can we maybe setup
skype call to discuss this and post summary here? Craig? Peter?

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




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

Re: [HACKERS] UPDATE of partition key

[Amit Langote]

On 2017/02/26 4:01, David G. Johnston wrote:
> IIUC ​it is already possible, for those who care to do so, to get a
> serialization failure in this scenario by upgrading isolation to repeatable
> read.

Maybe, this can be added as a note in the documentation.

Thanks,
Amit




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

Re: [HACKERS] user mapping messages

[Ashutosh Bapat]

On Thu, Feb 23, 2017 at 8:06 PM, Andrew Dunstan
 wrote:
>
> While reviewing the IF NOT EXISTS patch for CREATE USER MAPPING I
> noticed that in several places we treat the user name as the name of the
> user mapping. Strictly ISTM that user mappings are really anonymous
> objects, so instead of something like user "mapping \"%s\" does not
> exist for the server" we should possibly have "user mapping for user
> \"%s\" does not exist for the server".

Your proposed usage is better than the existing one.

> I was about to make that change
> in the patch when I saw that it was consistent with current usage. Do we
> want to stick with the current usage where we treat the user name as the
> mapping name, or change it?
>

We should change existing usage and then commit the patch with new
usage. The new message being added should be consistent with other
places.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2017-03-02 1:12 GMT+01:00 Alvaro Herrera :

>
> I've been giving this a look.  I started by tweaking the docs once
> again, and while verifying that the example works as expected, I
> replayed what I have in sgml:
>
> ... begin SGML paste ...
> 
>  For example, given the following XML document:
>   
>
>  the following query produces the result shown below:
>
> 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Thu, Mar 2, 2017 at 1:23 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> That is accurate. The only positive it has is that the user only
>> experiences one error, and it's the first error that was encountered if
>> reading top-to-bottom, left to right. It is an issue of which we
>> prioritize
>> - user experience or simpler code.
>>
>
> Hmmm. The last simpler structure I suggested, which is basically the one
> used in your code before the update, does check for the structure error
> first. The only drawback is that the condition is only evaluated when
> needed, which is an issue we can cope with, IMO.


Tom was pretty adamant that invalid commands are not executed. So in a case
like this, with ON_ERROR_STOP off:

\if false
\echo 'a'
\elif true
\echo 'b'
\elif invalid
\echo 'c'
\endif

Both 'b' and 'c' should print, because "\elif invalid" should not execute.
The code I had before was simpler, but it missed that.


>
> Now if you want to require committer opinion on this one, fine with me.
>>>
>>
>> Rather than speculate on what a committer thinks of this edge case (and
>> making a patch for each possible theory), I'd rather just ask them what
>> their priorities are and which user experience they favor.
>>
>
> ISTM that the consistent message by Robert & Tom was to provide simpler
> code even if the user experience is somehow degraded, as they required that
> user-friendly features were removed (eg trying to be nicer about structural
> syntax errors, barking in interactive mode so that the user always knows
> the current status, providing a detailed status indicator in the prompt...).
>

Ok, so here's one idea I tossed around, maybe this will strike the right
balance for you.

If I create a function like this:

static boolean
is_valid_else_context(IfState if_state, const char *cmd)
{
/* check for invalid \else / \elif contexts */

switch (if_state)

{
case IFSTATE_NONE:
/* not in an \if block */
psql_error("\\%s: no matching \\if\n", cmd);
return false;
break;
case IFSTATE_ELSE_TRUE:
case IFSTATE_ELSE_FALSE:
psql_error("\\%s: cannot occur after \\else\n", cmd);
return false;
break;
default:
break;
}
return true;
}



Then the elif block looks something like this:

else if (strcmp(cmd, "elif") == 0)
{
ifState if_state = conditional_stack_peek(cstack);

if (is_valid_else_context(if_state, "elif"))
{
/*
 * valid \elif context, check for valid expression
 */
bool elif_true = false;
success = read_boolean_expression(scan_state, "\\elif ",
_true);
if (success)
{
/*
 * got a valid boolean, what to do with it depends on
current
 * state
 */
switch (if_state)
{
case IFSTATE_IGNORED:
/*
 * inactive branch, do nothing.
 * either if-endif already had a true block,
 * or whole parent block is false.
 */
break;
case IFSTATE_TRUE:
/*
 * just finished true section of this if-endif,
 * must ignore the rest until \endif
 */
conditional_stack_poke(cstack, IFSTATE_IGNORED);
break;
case IFSTATE_FALSE:
/*
 * have not yet found a true block in this if-endif,
 * this might be the first.
 */
if (elif_true)
conditional_stack_poke(cstack, IFSTATE_TRUE);
break;
default:
/* error cases all previously ruled out */
break;
}
}
}
else
success = false;
psql_scan_reset(scan_state);
}


This is functionally the same as my latest patch, but the ugliness of
switching twice on if_state is hidden.

As an added benefit, the "else"-handling code gets pretty simple because it
can leverage that same function.

Does that handle your objections?

p.s.  do we try to avoid constructs likeif (success = my_function(var1,
var2))   ?

Re: [HACKERS] multivariate statistics (v24)

[Kyotaro HORIGUCHI]

Hello,

At Thu, 2 Mar 2017 04:05:34 +0100, Tomas Vondra  
wrote in 
> OK,
> 
> attached is v24 of the patch series, addressing most of the reported
> issues and comments (at least I believe so). The main changes are:

Unfortunately, 0002 conflicts with the current master
(4461a9b). Could you rebase them or tell us the commit where this
patches stand on?

I only saw the patch files but have some comments.

> 1) I've mostly abandoned the "multivariate" name in favor of
> "extended", particularly in places referring to stats stored in the
> pg_statistic_ext in general. "Multivariate" is now used only in places
> talking about particular types (e.g. multivariate histograms).
> 
> The "extended" name is more widely used for this type of statistics,
> and the assumption is that we'll also add other (non-multivariate)
> types of statistics - e.g. statistics on custom expressions, or some
> for of join statistics.

In 0005, and 

@@ -184,14 +208,43 @@ clauselist_selectivity(PlannerInfo *root,
 * If there are no such stats or not enough attributes, don't waste time
 * simply skip to estimation using the plain per-column stats.
 */
+   if (has_stats(stats, STATS_TYPE_MCV) &&
...
+   /* compute the multivariate stats */
+   s1 *= clauselist_ext_selectivity(root, mvclauses, stat);

@@ -1080,10 +1136,71 @@ clauselist_ext_selectivity_deps(PlannerInfo *root, 
Index relid,
 }
 
 /*
+ * estimate selectivity of clauses using multivariate statistic

These comment is left unchanged?  or on purpose? 0007 adds very
similar texts.

> 2) Catalog pg_mv_statistic was renamed to pg_statistic_ext (and
> pg_mv_stats view renamed to pg_stats_ext).

FWIW, "extended statistic" would be abbreviated as
"ext_statistic" or "extended_stats". Why have you exchanged the
words?

> 3) The structure of pg_statistic_ext was changed as proposed by
> Alvaro, i.e. the boolean flags were removed and instead we have just a
> single "char[]" column with list of enabled statistics.
> 
> 4) I also got rid of the "mv" part in most variable/function/constant
> names, replacing it by "ext" or something similar. Also mvstats.h got
> renamed to stats.h.
> 
> 5) Moved the files from src/backend/utils/mvstats to
> backend/statistics.
> 
> 6) Fixed the n_choose_k() overflow issues by using the algorithm
> proposed by Dean. Also, use the simple formula for num_combinations().
> 
> 7) I've tweaked data types for a few struct members (in stats.h). I've
> kept most of the uint32 fields at the top level though, because int16
> might not be large enough for large statistics and the overhead is
> minimal (compared to the space needed e.g. for histogram buckets).

Some formulated proof or boundary value test cases might be
needed (to prevent future trouble). Or any defined behavior on
overflow of them might be enough. I belive all (or most) of
overflow-able data has such behavior.

> The renames/changes were quite widespread, but I've done my best to
> fix all the comments and various other places.
> 
> regards

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

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

[Andres Freund]

On 2017-03-02 04:47:13 +0100, Tomas Vondra wrote:
> On 03/01/2017 11:55 PM, Andres Freund wrote:
> > On 2017-02-28 20:29:36 -0800, Andres Freund wrote:
> > > On 2017-02-28 20:18:35 -0800, Andres Freund wrote:
> > > > - Andres, hoping the buildfarm turns greener
> > > 
> > > Oh well, that didn't work. Investigating.
> > 
> > The fix for that was fairly trivial, and the buildfarm has cooled down.
> > 
> > The issue was that on 32bit platforms the Datum returned by some
> > functions (int2int4_sum in this case) isn't actually a separately
> > allocated Datum, but rather just something embedded in a larger
> > struct.  That, combined with the following code:
> > if (!peraggstate->resulttypeByVal && !*isnull &&
> > !MemoryContextContains(CurrentMemoryContext,
> >
> > DatumGetPointer(*result)))
> > seems somewhat problematic to me.  MemoryContextContains() can give
> > false positives when used on memory that's not a distinctly allocated
> > chunk, and if so, we violate memory lifetime rules.  It's quite
> > unlikely, given the required bit patterns, but nonetheless it's making
> > me somewhat uncomfortable.
> > 
> 
> I assume you're only using that code snippet as an example of code that
> might be broken by MemoryContextContains() false positives, right?

I'm mentioning that piece of code because it's what temporarily caused
all 32bit animals to fail, when I had made MemoryContextContains() less
forgiving.


> (I don't see how the slab allocator could interfere with aggregates, as it's
> only used for reorderbuffer.c).

Indeed, this is independent of slab.c. I just came across it because I
triggered crashes when shrinking the StandardChunkHeader to be just the
chunk's MemoryContext.


> > Do others think this isn't an issue and we can just live with it?
> > 
> 
> My understanding is all the places calling MemoryContextContains() assume
> they can't receive memory not allocated as a simple chunk by palloc(). If
> that's not the case, it's likely broken.

Yea, that's my conclusion too. Which means nodeAgg.c and nodeWindowAgg.c
are broken afaics, because of e.g. int2int4_sum's() use of
Int64GetDatumFast() on sub-parts of larger allocations.

- Andres


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


That is accurate. The only positive it has is that the user only
experiences one error, and it's the first error that was encountered if
reading top-to-bottom, left to right. It is an issue of which we prioritize
- user experience or simpler code.


Hmmm. The last simpler structure I suggested, which is basically the one 
used in your code before the update, does check for the structure error 
first. The only drawback is that the condition is only evaluated when 
needed, which is an issue we can cope with, IMO.



Now if you want to require committer opinion on this one, fine with me.


Rather than speculate on what a committer thinks of this edge case (and
making a patch for each possible theory), I'd rather just ask them what
their priorities are and which user experience they favor.


ISTM that the consistent message by Robert & Tom was to provide simpler 
code even if the user experience is somehow degraded, as they required 
that user-friendly features were removed (eg trying to be nicer about 
structural syntax errors, barking in interactive mode so that the user 
always knows the current status, providing a detailed status indicator in 
the prompt...).


Now committers can change their opinions, it is their privilege:-)

--
Fabien.


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

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 23 February 2017 at 16:02, Amit Langote
 wrote:
>
>> 2. In the patch, as part of the row movement, ExecDelete() is called
>> followed by ExecInsert(). This is done that way, because we want to
>> have the ROW triggers on that (sub)partition executed. If a user has
>> explicitly created DELETE and INSERT BR triggers for this partition, I
>> think we should run those. While at the same time, another question
>> is, what about UPDATE trigger on the same table ? Here again, one can
>> argue that because this UPDATE has been transformed into a
>> DELETE-INSERT, we should not run UPDATE trigger for row-movement. But
>> there can be a counter-argument. For e.g. if a user needs to make sure
>> about logging updates of particular columns of a row, he will expect
>> the logging to happen even when that row was transparently moved. In
>> the patch, I have retained the firing of UPDATE BR trigger.
>
> What of UPDATE AR triggers?

I think it does not make sense running after row triggers in case of
row-movement. There is no update happened on that leaf partition. This
reasoning can also apply to BR update triggers. But the reasons for
having a BR trigger and AR triggers are quite different. Generally, a
user needs to do some modifications to the row before getting the
final NEW row into the database, and hence [s]he defines a BR trigger
for that. And we can't just silently skip this step only because the
final row went into some other partition; in fact the row-movement
itself might depend on what the BR trigger did with the row. Whereas,
AR triggers are typically written for doing some other operation once
it is made sure the row is actually updated. In case of row-movement,
it is not actually updated.

>
> As a comment on how row-movement is being handled in code, I wonder if it
> could be be made to look similar structurally to the code in ExecInsert()
> that handles ON CONFLICT DO UPDATE.  That is,
>
> if (partition constraint fails)
> {
> /* row movement */
> }
> else
> {
> /* ExecConstraints() */
> /* heap_update(), EvalPlanQual(), and ExecInsertIndexTuples() */
> }

I guess this is what has been effectively done for row movement, no ?

Looking at that, I found that in the current patch, if there is no
row-movement happening, ExecPartitionCheck() effectively gets called
twice : First time when ExecPartitionCheck() is explicitly called for
row-movement-required check, and second time in ExecConstraints()
call. May be there should be 2 separate functions
ExecCheckConstraints() and ExecPartitionConstraints(), and also
ExecCheckConstraints() that just calls both. This way we can call the
appropriate functions() accordingly in row-movement case, and the
other callers would continue to call ExecConstraints().

>
> I see that ExecConstraint() won't get called on the source partition's
> constraints if row movement occurs.  Maybe, that's unnecessary because the
> new row won't be inserted into that partition anyway.

Yes I agree.

>
> ExecWithCheckOptions() for RLS update check does happen *before* row
> movement though.

Yes. I think we should do it anyways.

>
>> 3. In case of a concurrent update/delete, suppose session A has locked
>> the row for deleting it. Now a session B has decided to update this
>> row and that is going to cause row movement, which means it will
>> delete it first. But when session A is finished deleting it, session B
>> finds that it is already deleted. In such case, it should not go ahead
>> with inserting a new row as part of the row movement. For that, I have
>> added a new parameter 'already_delete' for ExecDelete().
>
> Makes sense.  Maybe: already_deleted -> concurrently_deleted.

Right, concurrently_deleted sounds more accurate. In the next patch, I
will change that.

>
>> Of course, this still won't completely solve the concurrency anomaly.
>> In the above case, the UPDATE of Session B gets lost. May be, for a
>> user that does not tolerate this, we can have a table-level option
>> that disallows row movement, or will cause an error to be thrown for
>> one of the concurrent session.
>
> Will this table-level option be specified for a partitioned table once or
> for individual partitions?

My opinion is, if decide to have table-level option, it should be on
the root partition, to keep it simple.

>
>> 4. The ExecSetupPartitionTupleRouting() is re-used for routing the row
>> that is to be moved. So in ExecInitModifyTable(), we call
>> ExecSetupPartitionTupleRouting() even for UPDATE. We can also do this
>> only during execution time for the very first time we find that we
>> need to do a row movement. I will think over that, but I am thinking
>> it might complicate things, as compared to always doing the setup for
>> UPDATE. WIll check on that.
>
> Hmm.  ExecSetupPartitionTupleRouting(), which does significant amount of
> setup work, is fine being called in ExecInitModifyTable() in the insert
> case because there are often cases where that's 

Re: [HACKERS] dropping partitioned tables without CASCADE

[Ashutosh Bapat]

>>
>> Isn't list_range_parted multilevel partitioned table. It gets dropped
>> in the testcases. So, I guess, we already have a testcase there.
>
> I thought Simon meant the test case where a partition that is itself
> partitioned is dropped.  At least that's what I took from "... fails *on*
> partition that has partitions".  So in the example I posted, drop table p1.

Ok. Thanks for the explanation.

>
> Anyway, there might be the confusion that *only* the root level
> partitioned table is of RELKIND_PARTITIONED_TABLE.  That's not true - any
> partitioned table (even one that's a partition) is of that relkind.  So
> the condition in the call to StoreCatalogInheritance1() is correct.  The
> following hunk:
>
> @@ -10744,7 +10756,9 @@ CreateInheritance(Relation child_rel, Relation
> parent_rel)
>  StoreCatalogInheritance1(RelationGetRelid(child_rel),
>   RelationGetRelid(parent_rel),
>   inhseqno + 1,
> - catalogRelation);
> + catalogRelation,
> + parent_rel->rd_rel->relkind ==
> +RELKIND_PARTITIONED_TABLE);
>
> Thanks,
> Amit
>
>

I agree.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

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

[Andres Freund]

On 2017-03-02 04:36:23 +0100, Tomas Vondra wrote:
> I've noticed two minor typos:
> 
> 1) That is solved this by creating ...
>- extra "this"
> 
> 2) Given this, routines like pfree their corresponding context ...
>- missing "find" or "determine"

Will fix.

> I also see you've explicitly mentioned the callbacks were added in 9.5.
> Doesn't that somewhat reintroduce the historical account?

That section I just moved up, the version reference was there before. I
left it in, because it seemed new enough to still be somewhat
relevant; removed and added it, not sure what's better.

Greetings,

Andres Freund


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

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

[Andres Freund]

On 2017-03-01 19:25:23 -0600, Jim Nasby wrote:
> On 2/28/17 11:21 AM, Andreas Karlsson wrote:
> > The only downside I can see to this approach is that we no logner will
> > able to reindex catalog tables concurrently, but in return it should be
> > easier to confirm that this approach can be made work.
> 
> Another downside is any stored regclass fields will become invalid.
> Admittedly that's a pretty unusual use case, but it'd be nice if there was
> at least a way to let users fix things during the rename phase (perhaps via
> an event trigger).

I'm fairly confident that we don't want to invoke event triggers inside
the CIC code...  I'm also fairly confident that between index oids
stored somewhere being invalidated - what'd be a realistic use case of
that - and not having reindex concurrently, just about everyone will
choose the former.

Regards,

Andres


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

Re: [HACKERS] Possible spelling fixes

[Josh Soref]

0001-spelling-comments.patch
Description: Binary data


0002-spelling-strings.patch
Description: Binary data


0003-spelling-sgml.patch
Description: Binary data


0004-spelling-variables.patch
Description: Binary data


0005-spelling-misc.patch
Description: Binary data

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

Re: [HACKERS] Radix tree for character conversion

[Kyotaro HORIGUCHI]

At Wed, 1 Mar 2017 14:34:23 +0900, Michael Paquier  
wrote in 
> On Tue, Feb 28, 2017 at 5:34 PM, Kyotaro HORIGUCHI
>  wrote:
> > At Tue, 28 Feb 2017 15:20:06 +0900, Michael Paquier 
> >  wrote in 
> > 
> >> +conv.o: conv.c char_converter.c
> >> This also can go away.
> >
> > Touching char_converter.c will be ignored if it is removed. Did
> > you mistake it for map_checker?
> 
> That was not what I meant: as pg_mb_radix_conv() is only used in
> conv.c, it may be better to just remove completely char_converter.c.

Ouch! You're right. Sorry for my short-sight. char_converter.c is
removed and related description in Makefile is removed.

> > And the code-comment pointed in the comment by the previous mail
> > is rewritten as Robert's suggestion.
> 
> Fine for me.
> 
> -distclean: clean
> +distclean:
> rm -f $(TEXTS)
> 
> -maintainer-clean: distclean
> -   rm -f $(MAPS)
> -
> +maintainer-clean:
> +   rm -f $(TEXTS) $(MAPS)
> Well, I would have assumed that this should not change..

I should have reverted there but actually the patch somehow does
the different thing.. Surely reverted it this time.

> The last version of the patch looks in rather good shape to me, we are
> also sure that the sanity checks on the old maps and the new maps
> match per the previous runs with map_checker.

Agreed.

>  One thing that still
> need some extra opinions is what to do with the old maps:
> 1) Just remove them, replacing the old maps by the new radix tree maps.
> 2) Keep them around in the backend code, even if they are useless.
> 3) Use a GUC to be able to switch from one to the other, giving a
> fallback method in case of emergency.
> 4) Use an extension module to store the old maps with as well the
> previous build code, so as sanity checks can still be performed on the
> new maps.
> 
> I would vote for 2), to reduce long term maintenance burdens and after
> seeing all the sanity checks that have been done in previous versions.

I don't vote 3 and 4. And I did 1 in the last patch.

About 2, any change in the authority files rarely but possiblly
happens. Even in the case the plain map files are no longer can
be generated. (but can with a bit tweak of convutils.pm) If the
radix-tree file generator is under a suspicion, the "plain" map
file generator (and the map_checker) or some other means to
sanity check might be required.

That being said, when something occurs in radix files, we can
find it in the radix file and can find the corresponding lines in
the aurhority file. The remaining problem is the case where some
substantial change in authority files doesn't affect radix
files. We can detect such mistake by detecting changes in
authority files. So I propose the 5th option.

5) Just remove plain map files and all related code. Addition to
   that, Makefile stores hash digest of authority files in
   Unicode/authoriy_hashes.txt or something that is managed by
   git.

This digest may differ among platforms (typically from cr/nl
difference) but we can assume *nix for the usage.

I will send the next version after this discussion is settled.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] SerializedSnapshotData alignment

[Noah Misch]

On Mon, Feb 27, 2017 at 12:11:54PM +0530, Robert Haas wrote:
> On Mon, Feb 27, 2017 at 5:13 AM, Noah Misch  wrote:
> > Dear 7b4ac19 authors,
> >
> > Field ps_snapshot_data usually receives four-byte alignment within
> > ParallelIndexScanDescData, but it contains the eight-byte whenTaken field.
> > The select_parallel test dies with SIGBUS on "Oracle Solaris 10 1/13
> > s10s_u11wos_24a SPARC", building with gcc 4.9.2.  Some credible fixes:
> >
> > 1. Move the SerializedSnapshotData declaration from snapmgr.c to snapmgr.h 
> > and
> >declare the ps_snapshot_data field to be of type SerializedSnapshotData.
> >Probably also add a field "TransactionId xids[FLEXIBLE_ARRAY_MEMBER]" to
> >SerializedSnapshotData, to assert the variable-length nature.
> >
> > 2. Change "char ps_snapshot_data[FLEXIBLE_ARRAY_MEMBER]" to "int64 ...".  I
> >have attached this in SerializedSnapshot-int64-v1.patch.
> >
> > 3. Change no declarations, and make snapmgr.c memcpy() the
> >SerializedSnapshotData through a local buffer.  I have attached this as
> >SerializedSnapshot-memcpy-v1.patch.
> >
> > I like (2) well enough, but I don't see that technique used elsewhere in the
> > tree.  (1) is more typical of PostgreSQL, though I personally like it when
> > structs can stay private to a file.  (3) is also well-attested, particularly
> > in xlog replay code.  I am leaning toward (2).  Other opinions?
> 
> In my imagination, we were already doing #3, so I'd probably favor
> that approach.

Pushed that way.


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

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Ashutosh Bapat]

Updated 0001 patch with some more comments. Attaching all the patches
for quick access.

On Wed, Mar 1, 2017 at 2:26 PM, Ashutosh Bapat
 wrote:
>>
>> 2. If the PartitionJoinPath emerges as the best path, we create paths
>> for each of the remaining child-joins. Then we collect paths with
>> properties same as the given PartitionJoinPath, one from each
>> child-join. These paths are converted into plans and a Merge/Append
>> plan is created combing these plans. The paths and plans for
>> child-join are created in a temporary memory context. The final plan
>> for each child-join is copied into planner's context and the temporary
>> memory context is reset.
>>
>
> Robert and I discussed this in more detail. Path creation code may
> allocate objects other than paths. postgres_fdw, for example,
> allocates character array to hold the name of relation being
> pushed-down. When the temporary context gets zapped after creating
> paths for a given child-join, those other objects also gets thrown
> away. Attached patch has implemented the idea that came out of the
> discussion.
>
> We create a memory context for holding paths at the time of creating
> PlannerGlobal and save it in PlannerGlobal. The patch introduces a new
> macro makePathNode() which allocates the memory for given type of path
> from this context. Every create_*_path function has been changed to
> use this macro instead of makeNode(). In standard_planner(), at the
> end of planning we destroy the memory context freeing all the paths
> allocated. While creating a plan node, planner copies everything
> required by the plan from the path, so the path is not needed any
> more. So, freeing corresponding memory should not have any adverse
> effects.
>
> Most of the create_*_path() functions accept root as an argument, thus
> the temporary path context is available through root->glob everywhere.
> An exception is create_append_path() which does not accept root as an
> argument. The patch changes create_append_path() and its callers like
> set_dummy_rel_pathlist(), mark_dummy_rel() to accept root as an
> argument. Ideally paths are not required after creating plan, so we
> should be
> able to free the context right after the call to create_plan(). But we
> need dummy paths while creating flat rtable in
> set_plan_references()->add_rtes_to_flat_rtable(). We used to So free
> the path context at the end of planning cycle. Now that we are
> allocating all the paths in a different memory context, it doesn't
> make sense to switch context in mark_dummy_rel().
>
> 0001 patch implements the idea described above.
> 0002 patch adds instrumentation to measure memory consumed in
> standard_planner() call.
> 0003 patch adds a GUC zap_paths to enable/disable destroying path context.
> The last two patches are for testing only.
>
> Attached also find the SQL script and its output showing the memory
> saved. For a 5 way self-join of pg_class, the total memory consumed in
> standard_planner() is 760K without patch and with patch it comes down
> to 713K, saving 47K memory otherwise occupied by paths. It looks like
> something useful even without partition-wise joins.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


0001-Free-up-memory-consumed-by-the-paths.patch
Description: Binary data


0002-Patch-to-measure-memory-used-in-CurrentMemoryContext.patch
Description: Binary data


0003-GUC-zap_path-to-enable-freeing-memory-consumed-by-pa.patch
Description: Binary data

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

Re: [HACKERS] \h tab-completion

[Kyotaro HORIGUCHI]

Hello,

At Wed, 1 Mar 2017 08:47:15 -0500, Peter Eisentraut 
 wrote in 

> On 2/3/17 07:12, Andreas Karlsson wrote:
> > On 01/25/2017 07:13 AM, Michael Paquier wrote:
> >> What I think you should do is making the code path of
> >> \\h smarter with some exceptions by using TailMatchesCS2() for ALTER.
> >> There is as well the case of DROP commands that should be treated by
> >> the way.
> > 
> > Yes, I think that is correct approach. I have attached a patch where I 
> > add completion for \h ALTER and \h DROP.
> 
> Instead of creating another copy of list_ALTER, let's use the
> words_after_create list and write a version of
> create_command_generator/drop_command_generator.

I'd like to separate the completion code into context-detector
and completion-engine. The former returns a "perse context value"
and the latter shows the suggestions from the values. Help engine
can use the same context-detector with the completion code. But
the correspondence between the two routines seems hardly
maintained:( (and such separation(refactoring) will be stuck on
the way)


Even this is a bit different topic from this patch aims, another
random thought on the help is that \h command should offer the
restriction words instead of all documents of possiblly-matching
(or head-matching) commands. For example, the current \h command
shows the following for create command.

=# \h create
Command: CREATE ACCESS METHOD
Description: define a new access method
Syntax:
CREATE ACCESS METHOD name
TYPE access_method_type
HANDLER handler_function

Command: CREATE AGGREGATE
...

This seems pointless to me and should offer the list of the next
words and short summary. gdb does the following for the case the
following.

| (gdb) help enable 
| Enable some breakpoints.
| Give breakpoint numbers (separated by spaces) as arguments.
| With no subcommand, breakpoints are enabled until you command otherwise.
| This is used to cancel the effect of the "disable" command.
| With a subcommand you can enable temporarily.
| 
| List of enable subcommands:
| 
| enable breakpoints -- Enable some breakpoints
| enable count -- Enable breakpoints for COUNT hits
| enable delete -- Enable breakpoints and delete when hit

| =# \h create
| Define an object
| 
| List of create subcommands:
| 
| CREATE ACCESS METHOD - Define a new access method
| CREATE AGGREGATE  - Define a new aggregate function
| ...


One bothersome problem is distinction between "CREATE TABLE" and
("CREATE TABLE AS" and "CREATE TABLESPACE"), but this might be
resolved by a small craft in documentation.

| - Documentation for "CREATE TABLE"
| Define a table
| 
| You might want to see the following commands.
| CREATE TABLE AS - Define a new table from the result of a query
| CREATE TABLESPACE - Define a new tablespace.
| 
| Syntax:
|  CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT 
EXISTS ] %s ( [

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] SerializedSnapshotData alignment

[Noah Misch]

On Mon, Feb 27, 2017 at 03:08:35PM +1300, Thomas Munro wrote:
> On Mon, Feb 27, 2017 at 2:18 PM, Noah Misch  wrote:
> > I wondered the same thing; if nothing else, why don't protosciurus and
> > castoroides fail the same way?  They do use older compilers, "Sun C 5.10
> > SunOS_sparc 2009/06/03" and gcc 3.4.3.  I have "Sun C 5.12 SunOS_sparc
> > 2011/11/16" and gcc 4.9.2, both of which are alignment-sensitive in several
> > configurations, according to the attached test program.  However, in a 
> > 32-bit
> > build with this Sun C, I don't get alignment-related bus errors.  (Those
> > animals build 64-bit, so this isn't the full story.)
> 
> It's been a while but I seem to recall that Sun C defaulted to a
> -xmemalign setting that tolerated misaligned reads in 32 bit builds,
> but used a different default on 64 bit builds.  "Solaris Application
> Programming" 5.8.5 seems to confirm this:  "For 32-bit applications,
> since Sun Studio 9, the default is for the compiler to assume 8-byte
> alignment and to trap and correct any misaligned data accesses.  For
> 64-bit applications, the compiler assumes 8-byte alignment, but the
> application will SIGBUS on a misaligned access."
> 
> Yet castoroides seems to be building with -m64, so that's not the
> explanation.  Could it be correctly aligned by coincidence?

Coincidental alignment was it.  ps_snapshot_data has offset 16 in a 64-bit
build and offset 12 in a 32-bit build, so it is well-aligned in 64-bit only.
I was building a 32-bit PostgreSQL; when I build a 64-bit PostgreSQL, I no
longer get the SIGBUS.


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

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

[Tomas Vondra]

On 03/01/2017 11:55 PM, Andres Freund wrote:

On 2017-02-28 20:29:36 -0800, Andres Freund wrote:

On 2017-02-28 20:18:35 -0800, Andres Freund wrote:

- Andres, hoping the buildfarm turns greener


Oh well, that didn't work. Investigating.


The fix for that was fairly trivial, and the buildfarm has cooled down.

The issue was that on 32bit platforms the Datum returned by some
functions (int2int4_sum in this case) isn't actually a separately
allocated Datum, but rather just something embedded in a larger
struct.  That, combined with the following code:
if (!peraggstate->resulttypeByVal && !*isnull &&
!MemoryContextContains(CurrentMemoryContext,
   
DatumGetPointer(*result)))
seems somewhat problematic to me.  MemoryContextContains() can give
false positives when used on memory that's not a distinctly allocated
chunk, and if so, we violate memory lifetime rules.  It's quite
unlikely, given the required bit patterns, but nonetheless it's making
me somewhat uncomfortable.



I assume you're only using that code snippet as an example of code that 
might be broken by MemoryContextContains() false positives, right?


(I don't see how the slab allocator could interfere with aggregates, as 
it's only used for reorderbuffer.c).


>

Do others think this isn't an issue and we can just live with it?



My understanding is all the places calling MemoryContextContains() 
assume they can't receive memory not allocated as a simple chunk by 
palloc(). If that's not the case, it's likely broken.


regards

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


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

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

[David Steele]

On 3/1/17 5:11 PM, Jim Nasby wrote:
> On 2/27/17 6:25 PM, David Steele wrote:
>> The purpose of this patch is to make waiting for archive optional, with
>> the default being the current behavior, i.e., to wait for all WAL to be
>> archived.  This functionality is already used internally by
>> pg_basebackup, so the only real change is to expose it through the
>> pg_stop_backup() function.
> 
> Do the docs mention anywhere how to monitor WAL archiving to know if
> you've got all the necessary WAL? Perhaps a function to do that would be
> worth adding.

The docs in the patch mention that this option should only be used by
backup solutions that know how to monitor archiving:

+This behavior is only useful for backup
+software which independently monitors WAL archiving, otherwise WAL
+required to make the backup consistent might be missing and make
the backup
+useless.

There is already a view, pg_stat_archiver, that allows a program to see
what has been archived.  I'm not sure we'd be adding much by putting a
function around that.

I would be OK with adding a link to pg_stat_archiver in the
pg_stop_backup() documentation to if that seems appropriate.

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


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

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

[Tomas Vondra]

On 03/01/2017 05:18 AM, Andres Freund wrote:

On 2017-02-28 10:41:22 -0800, Andres Freund wrote:

Hi,

On 2017-02-27 23:44:20 -0800, Andres Freund wrote:

*preliminary* patch attached. This needs a good bit of polishing
(primarily comment work, verifying that valgrind works), but I'm too
tired now.

I'm not quite sure how to deal with mmgr/README - it's written as kind
of a historical document, and the "Mechanisms to Allow Multiple Types of
Contexts" is already quite out of date.  I think it'd be good to rip out
all the historical references and just describe the current state, but
I'm not really enthusiastic about tackling that :/


While still not enthusiastic, I took a stab at doing so.  While still
not perfect, I do think this is an improvement.

Is anybody uncomfortable going away from the current historical account
style?


I've pushed these now. I'm not claiming that the README revision is
perfect, but we can incremently improve it further...



Thanks. I went through the README and it definitely looks better now.

I've noticed two minor typos:

1) That is solved this by creating ...
   - extra "this"

2) Given this, routines like pfree their corresponding context ...
   - missing "find" or "determine"

I also see you've explicitly mentioned the callbacks were added in 9.5. 
Doesn't that somewhat reintroduce the historical account?



regards

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


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

Re: [HACKERS] Patch to improve performance of replay of AccessExclusiveLock

[Amit Kapila]

On Wed, Mar 1, 2017 at 5:32 PM, David Rowley
 wrote:
> Hackers,
>
> I've attached a small patch which aims to improve the performance of
> AccessExclusiveLock when there are many AccessExclusiveLock stored.
>

I could see that your idea is quite straightforward to improve
performance (basically, convert recovery lock list to recovery lock
hash table based on xid), however, it is not clear under what kind of
workload this will be helpful?  Do you expect that many concurrent
sessions are trying to acquire different AEL locks?

Few comments on the patch:
1.
+/*
+ * Number of buckets to split RecoveryLockTable into.
+ * This must be a power of two.
+ */

+#define RECOVERYLOCKTABLE_SIZE 1024

On what basis did you decide to keep the lock table size as 1024, is
it just a guess, if so, then also adding a comment to indicate that
you think this is sufficient for current use cases seems better.
However, if it is based on some data, then it would be more
appropriate.

2.
@@ -607,6 +627,8 @@ StandbyAcquireAccessExclusiveLock(TransactionId
xid, Oid dbOid, Oid relOid)
 {
  xl_standby_lock *newlock;
  LOCKTAG locktag;
+ size_t pidx;
+

Comment on top of this function needs some change, it still seems to
refer to RelationLockList.

* We keep a single dynamically expandible list of locks in local memory,
 * RelationLockList, so we can keep track of the various entries made by
 * the Startup process's virtual xid in the shared lock table.

3.
+ size_t pidx;
+
+ if (!TransactionIdIsValid(xid))
+ {
+ StandbyReleaseAllLocks();
+ return;
+ }

What is the need of this new code?

4.
In some cases, it might slow down the performance where you have to
traverse the complete hash table like StandbyReleaseOldLocks, however,
I think those cases will be less relative to other lock release calls
which are called at every commit, so probably this is okay.


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


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

Re: [HACKERS] Transactions involving multiple postgres foreign servers

[vinayak]

On 2017/02/28 16:54, Masahiko Sawada wrote:


I've created a wiki page[1] describing about the design and
functionality of this feature. Also it has some examples of use case,
so this page would be helpful for even testing. Please refer it if
you're interested in testing this feature.

[1] 2PC on FDW


Thank you for creating the wiki page.

In the "src/test/regress/pg_regress.c" file
-* xacts.  (Note: to reduce the probability of 
unexpected shmmax
-* failures, don't set max_prepared_transactions any 
higher than

-* actually needed by the prepared_xacts regression test.)
+* xacts. We also set *max_fdw_transctions* to enable 
testing of atomic
+* foreign transactions. (Note: to reduce the 
probability of unexpected

+* shmmax failures, don't set max_prepared_transactions or
+* max_prepared_foreign_transactions any higher than 
actually needed by the

+* corresponding regression tests.).

I think we are not setting the "*max_fdw_transctions" *anywhere.
Is this correct?

In the "src/bin/pg_waldump/rmgrdesc.c" file following header file used 
two times.

+ #include "access/fdw_xact.h"
I think we need to remove one line.

Regards,
Vinayak Pokale

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[Kyotaro HORIGUCHI]

Hello,

At Tue, 28 Feb 2017 10:39:01 -0500, Stephen Frost  wrote in 
<20170228153901.gh9...@tamriel.snowman.net>
> * David Fetter (da...@fetter.org) wrote:
> > On Mon, Feb 27, 2017 at 11:53:17PM -0500, Stephen Frost wrote:
> > > * Kyotaro HORIGUCHI (horiguchi.kyot...@lab.ntt.co.jp) wrote:
> > > > I suppose it is for suggesting what kind of word should come
> > > > there, or avoiding silence for a tab. Or for symmetry with other
> > > > types of manipulation, like DROP. Another possibility is creating
> > > > multiple objects with similar names, say CREATE TABLE employee_x1,
> > > > CREATE TABLE employee_x2. Just trying to complete existing
> > > > *schema* is one more another possible objective.
> > > 
> > > I don't buy any of these arguments either.  I *really* don't want us
> > > going down some road where we try to make sure that hitting 'tab'
> > > never fails...

These suggestions exist before this patch. Whether to remove them
would be another discussion. I was going to add some but finally
I believe I have added no such things in this patchset.

> > Wouldn't that just be a correct, grammar-aware implementation of tab
> > completion?  Why wouldn't you want that?
> 
> No, it wouldn't, it would mean we have to provide something for cases
> where it doesn't make sense to try and provide an answer, as being
> discussed here for CREATE TABLE.
> 
> We can't provide an answer based on tab-completion to what you want to
> call your new table.

The difference seems to be that what we take this feature to
be. If we see it as just a fast-path of entering a word where we
know what words should come, silence is not a problem. If we see
it as safety-wheels to guide users to the right way to go,
silence would be bad. A silence during word completion suggests
something wrong in the preceding words to me so it is a bit
annoying.

As an analogous operation, mkdir on bash suggests existing
directories. We can suggest existing tables for CREATE TABLE with
the same basis.

Another possible way to go would be showing a 'suggestion' not a
list of possibilities. If readline allows such operation, I
imagine the following. But this is a quite diferrent discussion.

=# CREATE TABLE 
<>
=# CREATE TABLE table1 

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

[Michael Paquier]

On Wed, Mar 1, 2017 at 2:21 AM, Andreas Karlsson  wrote:
> For each table:
>
> 1. Create new indexes without populating them, and lock the tables and
> indexes for the session.

+/*
+ * Copy contraint flags for old index. This is safe because the old index
+ * guaranteed uniquness.
+ */
+newIndexForm->indisprimary = oldIndexForm->indisprimary;
+oldIndexForm->indisprimary = false;
+newIndexForm->indisexclusion = oldIndexForm->indisexclusion;
+oldIndexForm->indisexclusion = false;
[...]
+deleteDependencyRecordsForClass(RelationRelationId, newIndexOid,
+RelationRelationId, DEPENDENCY_AUTO);
+deleteDependencyRecordsForClass(RelationRelationId, oldIndexOid,
+ConstraintRelationId,
DEPENDENCY_INTERNAL);
+
+// TODO: pg_depend for old index?
There is a lot of mumbo-jumbo in the patch to create the exact same
index definition as the original one being reindexed, and that's a
huge maintenance burden for the future. You can blame me for that in
the current patch. I am wondering if it would not just be better to
generate a CREATE INDEX query string and then use the SPI to create
the index, and also do the following extensions at SQL level:
- Add a sort of WITH NO DATA clause where the index is created, so the
index is created empty, and is marked invalid and not ready.
- Extend pg_get_indexdef_string() with an optional parameter to
enforce the index name to something else, most likely it should be
extended with the WITH NO DATA/INVALID clause, which should just be a
storage parameter by the way.
By doing something like that what the REINDEX CONCURRENTLY code path
should just be careful about is that it chooses an index name that
avoids any conflicts.
-- 
Michael


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

Re: [HACKERS] Two questions about Postgres parser

[Jim Nasby]

On 2/27/17 10:37 AM, Tom Lane wrote:

2. Implicit user defined type casts are not applied for COALESCE operator:

That has nothing to do with whether the cast is user-defined.  It has to
do with not wanting to automatically unify types across type-category
boundaries (in this case, numeric vs. composite categories).  That's per
step 4 here:

https://www.postgresql.org/docs/devel/static/typeconv-union-case.html

and it's not an easy thing to get rid of because if you're considering
more than one type category then the heuristic about preferring "preferred
types" breaks down --- how do you know which category's preferred type to
prefer?


FWIW, while working on a variant type I wished there was a way to 
preempt built-in type resolution when dealing with a particular type. I 
was specifically interested in function calls, which IIRC is handled by 
a single function and a helper. Exporting those two and providing a hook 
would have done the trick in my case.

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


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

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

[Jim Nasby]

On 2/28/17 11:21 AM, Andreas Karlsson wrote:

The only downside I can see to this approach is that we no logner will
able to reindex catalog tables concurrently, but in return it should be
easier to confirm that this approach can be made work.


Another downside is any stored regclass fields will become invalid. 
Admittedly that's a pretty unusual use case, but it'd be nice if there 
was at least a way to let users fix things during the rename phase 
(perhaps via an event trigger).

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


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

Re: [HACKERS] Disallowing multiple queries per PQexec()

[Jim Nasby]

On 2/28/17 2:45 PM, Andres Freund wrote:

So if you don't want to allow multiple statements, use PQexecParams et
al.


That does leave most application authors out in the cold though, since 
they're using a higher level connection manager.


If the maintenance burden isn't terribly high it would be nice to allow 
disabling multiple statements via a GUC.

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


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

Re: [HACKERS] Wrong variable type in KeepLogSeg

[Kyotaro HORIGUCHI]

At Tue, 28 Feb 2017 12:21:01 +0100, Magnus Hagander  wrote 
in 
magnus> > Hello, I found a variable definition with wrong type
magnus> > specification in KeepLogSeg, which doesn't harm anything.
magnus> 
magnus> Nice catch. Applied and backpatched.

Thank you for committing.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] I propose killing PL/Tcl's "modules" infrastructure

[Jim Nasby]

On 2/27/17 2:42 PM, Tom Lane wrote:

+ SET pltcl.start_proc = 'no_such_function';
+ select tcl_int4add(1, 2);
+ ERROR:  function no_such_function() does not exist


Can the error message be more explicit somehow? Otherwise people will be 
quite confused as to where no_such_function() is coming from.



BTW, I'd think this functionality would be valuable for every PL. Maybe 
it's worth adding formal support for it to pg_language et all and leave 
it up to each language to decide whether it's supported or not? Multiple 
init functions might be useful too, similar to how we support multiple 
hook functions (though presumably a field of regproc[] is a better way 
to handle that...)


I'm also wondering if there'd be value to supporting code that runs on 
each function invocation.

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


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

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

[Jim Nasby]

On 2/27/17 6:25 PM, David Steele wrote:

The purpose of this patch is to make waiting for archive optional, with
the default being the current behavior, i.e., to wait for all WAL to be
archived.  This functionality is already used internally by
pg_basebackup, so the only real change is to expose it through the
pg_stop_backup() function.


Do the docs mention anywhere how to monitor WAL archiving to know if 
you've got all the necessary WAL? Perhaps a function to do that would be 
worth adding.

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


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

Re: [HACKERS] GSoC 2017

[Jim Nasby]

On 2/27/17 4:52 PM, Thomas Munro wrote:

By the way, that page claims that PostgreSQL runs on Irix and Tru64,
which hasn't been true for a few years.


There could be a GSoC project to add support for those back in... ;P
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

Re: [HACKERS] mat views stats

[Michael Paquier]

On Thu, Mar 2, 2017 at 7:20 AM, Jim Mlodgenski  wrote:
>
>
> On Sun, Feb 26, 2017 at 11:49 AM, Robert Haas  wrote:
>>
>> On Wed, Feb 22, 2017 at 11:13 AM, Jim Nasby 
>> wrote:
>> > Certainly easier, but I don't think it'd be better. Matviews really
>> > aren't
>> > the same thing as tables. Off-hand (without reviewing the patch), update
>> > and
>> > delete counts certainly wouldn't make any sense. "Insert" counts might,
>> > in
>> > as much as it's how many rows have been added by refreshes. You'd want a
>> > refresh count too.
>>
>> Regular REFRESH truncates the view and repopulates it, but REFRESH
>> CONCURRENTLY does inserts, updates, and deletes as needed to adjust
>> the contrs that make sense for
>> regular tables are also sensible here.
>>
>
> After digging into things further, just making refresh report the stats for
> what is it basically doing simplifies and solves it and it is something we
> can back patch if that the consensus. See the attached patch.

This is unhappy:
$ git diff master --check
src/backend/commands/matview.c:155: indent with spaces.
+uint64  processed = 0;

+/*
+ * Send the stats to mimic what we are essentially doing.
+ * A truncate and insert
+ */
This sentence is unfinished.

There is also no need to report the number of inserts if WITH NO DATA is used.
-- 
Michael


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

Re: [HACKERS] 2017-03 Commitfest In Progress

[Tom Lane]

Thomas Munro  writes:
> On Thu, Mar 2, 2017 at 4:42 AM, David Steele  wrote:
>> The 2017-03 commitfest is now in progress.  Here's the breakdown:
>> 
>> Needs review: 128
>> Waiting on Author: 26
>> Ready for Committer: 26
>> Total: 180

> Not quite a record haul but close.

Indeed.  As usual, a depressingly large number of patches appeared out of
the woodwork in the last few days before the deadline, and more than a
couple of those seem to be clear violations of our rule about "no major
new features should first appear during the last commitfest".

I think we should have a discussion about which patches need to be booted
to the next CF (ie, first of the v11 cycle) without further attention now.
ISTM it would be quite unfair if patches that have been in the queue for
much longer fail to get into v10 because johnny-come-lately patches
consumed reviewer and committer bandwidth.

regards, tom lane


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

Re: [HACKERS] port of INSTALL file generation to XSLT

[Magnus Hagander]

On Wed, Mar 1, 2017 at 3:58 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 2/28/17 08:57, Magnus Hagander wrote:
> > It appears we need pandoc 1.13 to get the good output.  This won't be
> > available until Debian stretch.
> >
> > So for PG10, I propose the attached revised patch which keeps using
> lynx
> > but uses xsltproc instead of jade.
> >
> >
> > It is available for people not using debian though? Might it be
> > worthwhile to make it dependent on the version of pandoc -- so use that
> > method if people have the newer pandoc and fall back to lynx if they
> don't?
>
> Well, this really only runs once every couple of months on borka and
> here or there for those building their own snapshot tarballs.  I don't
> think we need to cater to a wide audience here.  In fact, variety could
> be bad here:  We don't want to find out that a tarball was rolled with
> the wrong variant.
>

Good point. Let 's just go for it then.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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

[Craig Ringer]

On 2 March 2017 at 06:20, Petr Jelinek  wrote:

> If I understand you correctly you are saying that if PREPARE is being
> decoded, we can load the GID from the 2pc info in memory about the
> specific 2pc. The info gets removed on COMMIT PREPARED but at that point
> there is no real difference between replicating it as 2pc or 1pc since
> the 2pc behavior is for all intents and purposes lost at that point.
> Works for me. I guess the hard part is knowing if COMMIT PREPARED
> happened at the time PREPARE is decoded, but I existence of the needed
> info could be probably be used for that.

Right.


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


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

Re: [HACKERS] Restricting maximum keep segments by repslots

[Kyotaro HORIGUCHI]

At Wed, 1 Mar 2017 12:18:07 -0500, Peter Eisentraut 
 wrote in 
<98538b00-42ae-6a6b-f852-50b3c937a...@2ndquadrant.com>
> On 2/27/17 22:27, Kyotaro HORIGUCHI wrote:
> > This patch adds a GUC to put a limit to the number of segments
> > that replication slots can keep.
> 
> Please measure it in size, not in number of segments.

It was difficult to dicide which is reaaonable but I named it
after wal_keep_segments because it has the similar effect.

In bytes(or LSN)
 max_wal_size
 min_wal_size
 wal_write_flush_after

In segments
 wal_keep_segments

But surely max_slot_wal_keep_segments works to keep disk space so
bytes would be reasonable.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Restricting maximum keep segments by repslots

[Kyotaro HORIGUCHI]

At Wed, 1 Mar 2017 12:17:43 -0500, Peter Eisentraut 
 wrote in 

> On 2/27/17 23:27, Petr Jelinek wrote:
> >>> WARNING:  restart LSN of replication slots is ignored by checkpoint
> >>> DETAIL:  Some replication slots lose required WAL segnents to continue.
> > However this is dangerous as logical replication slot does not consider
> > it error when too old LSN is requested so we'd continue replication,
> > hiding data loss.
> 
> In general, we would need a much more evident and strict way to discover
> when this condition is hit.  Like a "full" column in
> pg_stat_replication_slot, and refusing connections to the slot until it
> is cleared.

Anyway, if preserving WAL to replicate has priority to the
master's health, this doesn't nothing by leaving
'max_wal_keep_segments' to 0.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Restricting maximum keep segments by repslots

[Kyotaro HORIGUCHI]

At Wed, 1 Mar 2017 08:06:10 -0800, Andres Freund  wrote in 
<20170301160610.wc7ez3vihmial...@alap3.anarazel.de>
> On 2017-02-28 12:42:32 +0900, Michael Paquier wrote:
> > Please no. Replication slots are designed the current way because we
> > don't want to have to use something like wal_keep_segments as it is a
> > wart, and this applies as well for replication slots in my opinion.
> 
> I think a per-slot option to limit the amount of retention would make
> sense.

I started from that but I found that all slots refer to the same
location as the origin of the distance, that is, the last segment
number that KeepLogSeg returns. As the result the whole logic
became as the following. This is one reason of the proposed pach.

- Take the maximum value of the maximum-retain-LSN-amount per slot.
- Apply the maximum value during the calcuation in KeepLogSeg.
- (These steps runs only when at least one slot exists)

The another reason was, as Robert retold, I thought that this is
a matter of system (or a DB cluster) wide health and works in a
bit different way from what the name "max_wal_size_hard"
suggests.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

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

[Kouhei Kaigai]

> Hello all,
> 
> as this is my first mail to pgsql-hackers, please be gentle :)
>
Welcome to pgsql-hackers,

> I've looked at the patch, and as I'm not that familiar with the pg-sourcecode,
> customs and so on, this isn't a review, more like food for thought and all
> should be taken with a grain of salt. :)
> 
> So here are a few questions and remarks:
> 
> >+double  limit_tuples = -1.0;
> 
> Surely the limit cannot be fractional, and must be an integer. So wouldn't
> it be better the same type as say:
> 
> >+if (root->limit_tuples >= 0.0 &&
> 
> Than you could also compare with ">= 0", not ">= 0.0".
>
The above variable represents the "estimated" number of rows at the
planning stage, not execution time.
You may be able to see Path structure has "rows" field declared as
double type. It makes sense to consider stupid paths during planning,
even if it is eventually rejected. For example, if a cross join with
two large tables appear during planning, 64bit integer will make overflow
easily.

> node->ss.ps.ps_numTuples is f.i. an uint64.
> 
> Or is there a specific reason the limit must be a double?
>
The above variable represents "actual" number of rows at the execution
stage. Likely, hardware replacement cycle will come before int64 overflow.

> And finally:
> 
> >+if (node->ss.ps.ps_numTuples > 0)
> 
> >+appendStringInfo(, " LIMIT %ld",
> node->ss.ps.ps_numTuples);
> 
> vs.
> 
> >+appendStringInfo(, "%s LIMIT %lu",
> >+ sql,
> node->ss.ps.ps_numTuples);
> 
> It seems odd to have two different format strings here for the same variable.
>
Ah, yes, %lu is right because ps_numTuples is uint64.

> A few comments miss "." at the end, like these:
> 
> >+ * Also, pass down the required number of tuples
> 
> >+ * Pass down the number of required tuples by the upper node
> 
OK,

> And this comment might be better "were we already called?"
> 
> >+boolrs_started; /* are we already
> called? */
> 
Other variables in ResultState uses present form, like:

+   boolrs_started; /* are we already called? */
boolrs_done;/* are we done? */
boolrs_checkqual;   /* do we need to check the qual? */
 } ResultState;

Thanks,

PG-Strom Project / NEC OSS Promotion Center
KaiGai Kohei 


passdown-limit-fdw.v5.patch
Description: passdown-limit-fdw.v5.patch

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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

I've been giving this a look.  I started by tweaking the docs once
again, and while verifying that the example works as expected, I
replayed what I have in sgml:

... begin SGML paste ...

 For example, given the following XML document:
  

 the following query produces the result shown below:

Re: [HACKERS] [PATCH] Use $ parameters as replacement characters for pg_stat_statements

[Lukas Fittl]

On Wed, Mar 1, 2017 at 6:51 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> Hmm, I think this could confuse people into thinking that the queries
> displayed were in fact prepared queries.
>
> Maybe we could gather some more ideas.
>

I think thats a reasonable concern - the main benefit of $1 is that its
already designated as something that can replace a constant, and still be
read by the Postgres parser.

Is there any other character that has the same properties?

I'll also note that Greg Stark mentioned in [0] that "There's another
feature pg_stat_statements *really* needs. A way to convert a jumbled
statement into one that can be prepared easily. The use of ? instead of :1
:2 etc makes this a mechanical but annoying process."

Using $1, $2, etc. for jumbling statements would give us that for free, no
additional effort needed.

[0] https://www.postgresql.org/message-id/CAM-w4HNOeNW6pY_1%
3DLp1aJGMmZ_R6S8JHjqvJMv8-%3DOf3q1q0w%40mail.gmail.com

Best,
Lukas

-- 
Lukas Fittl

Re: [HACKERS] pg_dump, pg_dumpall and data durability

[Michael Paquier]

On Thu, Mar 2, 2017 at 2:26 AM, David Steele  wrote:
> This patch is in need of a committer.  Any takers?
> I didn't see a lot of enthusiasm from committers on the thread

Stephen at least has showed interest.

> so if nobody picks it up by the end of the CF I'm going to mark the patch RWF.

Yes, that makes sense. If no committer is willing to have a look at
code-level and/or has room for this patch then it is as good as
doomed. Except for bug fixes, I have a couple of other patches that
are piling up so they would likely get the same treatment. There is
nothing really we can do about that.
-- 
Michael


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

Re: [HACKERS] timeouts in PostgresNode::psql

[Craig Ringer]

On 2 March 2017 at 03:19, Peter Eisentraut
 wrote:
> On 2/26/17 21:28, Craig Ringer wrote:
>> Amended patch attached after a few Perl-related comments I got on
>> private mail. Instead of
>>
>> $exc_save !~ /^$timeout_exception.*/
>>
>> I've updated to:
>>
>> $exc_save !~ /^\Q$timeout_exception\E/
>>
>> i.e. don't do an unnecessary wildcard match at the end, and disable
>> metachar interpretation in the substituted range.
>>
>> Still needs applying to pg9.6 and pg10.
>
> committed to master and 9.6

Thanks.



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


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

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

[Andres Freund]

On 2017-02-28 20:29:36 -0800, Andres Freund wrote:
> On 2017-02-28 20:18:35 -0800, Andres Freund wrote:
> > - Andres, hoping the buildfarm turns greener
> 
> Oh well, that didn't work. Investigating.

The fix for that was fairly trivial, and the buildfarm has cooled down.

The issue was that on 32bit platforms the Datum returned by some
functions (int2int4_sum in this case) isn't actually a separately
allocated Datum, but rather just something embedded in a larger
struct.  That, combined with the following code:
if (!peraggstate->resulttypeByVal && !*isnull &&
!MemoryContextContains(CurrentMemoryContext,
   
DatumGetPointer(*result)))
seems somewhat problematic to me.  MemoryContextContains() can give
false positives when used on memory that's not a distinctly allocated
chunk, and if so, we violate memory lifetime rules.  It's quite
unlikely, given the required bit patterns, but nonetheless it's making
me somewhat uncomfortable.

Do others think this isn't an issue and we can just live with it?

Regards,

Andres


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

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

[Petr Jelinek]

On 01/03/17 10:24, Craig Ringer wrote:
> On 9 February 2017 at 21:23, Stas Kelvich  wrote:
> 
>>> On 2 Feb 2017, at 00:35, Craig Ringer  wrote:
>>>
>>> Stas was concerned about what happens in logical decoding if we crash 
>>> between PREPSRE TRANSACTION and COMMIT PREPARED. But we'll always go back 
>>> and decode the whole txn again anyway so it doesn't matter.
>>
>> Not exactly. It seems that in previous discussions we were not on the same 
>> page, probably due to unclear arguments by me.
>>
>> From my point of view there is no problems (or at least new problems 
>> comparing to ordinary 2PC) with preparing transactions on slave servers with 
>> something like “#{xid}#{node_id}” instead of GID if issuing node is 
>> coordinator of that transaction. In case of failure, restart, crash we have 
>> the same options about deciding what to do with uncommitted transactions.
> 
> But we don't *need* to do that. We have access to the GID of the 2PC
> xact from PREPARE TRANSACTION until COMMIT PREPARED, after which we
> have no need for it. So we can always use the user-supplied GID.
> 
>> I performed some tests to understand real impact on size of WAL. I've 
>> compared postgres -master with wal_level = logical, after 3M 2PC 
>> transactions with patched postgres where GID’s are stored inside commit 
>> record too.
> 
> Why do you do this? You don't need to. You can look the GID up from
> the 2pc status table in memory unless the master already did COMMIT
> PREPARED, in which case you can just decode it as a normal xact as if
> it were never 2pc in the first place.
> 
> I don't think I've managed to make this point by description, so I'll
> try to modify your patch to demonstrate.
> 

If I understand you correctly you are saying that if PREPARE is being
decoded, we can load the GID from the 2pc info in memory about the
specific 2pc. The info gets removed on COMMIT PREPARED but at that point
there is no real difference between replicating it as 2pc or 1pc since
the 2pc behavior is for all intents and purposes lost at that point.
Works for me. I guess the hard part is knowing if COMMIT PREPARED
happened at the time PREPARE is decoded, but I existence of the needed
info could be probably be used for that.

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


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

Re: [HACKERS] mat views stats

[Jim Mlodgenski]

On Sun, Feb 26, 2017 at 11:49 AM, Robert Haas  wrote:

> On Wed, Feb 22, 2017 at 11:13 AM, Jim Nasby 
> wrote:
> > Certainly easier, but I don't think it'd be better. Matviews really
> aren't
> > the same thing as tables. Off-hand (without reviewing the patch), update
> and
> > delete counts certainly wouldn't make any sense. "Insert" counts might,
> in
> > as much as it's how many rows have been added by refreshes. You'd want a
> > refresh count too.
>
> Regular REFRESH truncates the view and repopulates it, but REFRESH
> CONCURRENTLY does inserts, updates, and deletes as needed to adjust
> the contents.  So I think all the same counters that make sense for
> regular tables are also sensible here.
>
>
After digging into things further, just making refresh report the stats for
what is it basically doing simplifies and solves it and it is something we
can back patch if that the consensus. See the attached patch.
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index a18c917..4383312 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -30,6 +30,7 @@
 #include "executor/spi.h"
 #include "miscadmin.h"
 #include "parser/parse_relation.h"
+#include "pgstat.h"
 #include "rewrite/rewriteHandler.h"
 #include "storage/lmgr.h"
 #include "storage/smgr.h"
@@ -59,7 +60,7 @@ static void transientrel_startup(DestReceiver *self, int operation, TupleDesc ty
 static bool transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
 static void transientrel_shutdown(DestReceiver *self);
 static void transientrel_destroy(DestReceiver *self);
-static void refresh_matview_datafill(DestReceiver *dest, Query *query,
+static uint64 refresh_matview_datafill(DestReceiver *dest, Query *query,
 		 const char *queryString);
 
 static char *make_temptable_name_n(char *tempname, int n);
@@ -151,6 +152,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	Oid			save_userid;
 	int			save_sec_context;
 	int			save_nestlevel;
+uint64  processed = 0;
 	ObjectAddress address;
 
 	/* Determine strength of lock needed. */
@@ -322,7 +324,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 
 	/* Generate the data, if wanted. */
 	if (!stmt->skipData)
-		refresh_matview_datafill(dest, dataQuery, queryString);
+		processed = refresh_matview_datafill(dest, dataQuery, queryString);
 
 	heap_close(matviewRel, NoLock);
 
@@ -345,8 +347,17 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 		Assert(matview_maintenance_depth == old_depth);
 	}
 	else
+{
 		refresh_by_heap_swap(matviewOid, OIDNewHeap, relpersistence);
 
+/* 
+ * Send the stats to mimic what we are essentially doing. 
+ * A truncate and insert 
+ */
+pgstat_count_truncate(matviewRel);
+pgstat_count_heap_insert(matviewRel, processed);
+}
+
 	/* Roll back any GUC changes */
 	AtEOXact_GUC(false, save_nestlevel);
 
@@ -361,7 +372,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 /*
  * refresh_matview_datafill
  */
-static void
+static uint64 
 refresh_matview_datafill(DestReceiver *dest, Query *query,
 		 const char *queryString)
 {
@@ -369,6 +380,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 	PlannedStmt *plan;
 	QueryDesc  *queryDesc;
 	Query	   *copied_query;
+uint64 processed;
 
 	/* Lock and rewrite, using a copy to preserve the original query. */
 	copied_query = copyObject(query);
@@ -406,6 +418,8 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 	/* run the plan */
 	ExecutorRun(queryDesc, ForwardScanDirection, 0L);
 
+processed = queryDesc->estate->es_processed;
+
 	/* and clean up */
 	ExecutorFinish(queryDesc);
 	ExecutorEnd(queryDesc);
@@ -413,6 +427,8 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 	FreeQueryDesc(queryDesc);
 
 	PopActiveSnapshot();
+
+return processed;
 }
 
 DestReceiver *

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

Re: [HACKERS] [BUGS] Seems bug in postgres_fdw?

[Rader, David]

On Mon, Feb 27, 2017 at 10:10 AM, Tom Lane  wrote:

> Sachin Kotwal  writes:
> > Here , Why postgresql takes different time when remote table and foreign
> > table have different definition for timestamp column?
>
> I believe postgres_fdw sets the timezone in its remote session to UTC
> for predictability of results.  Your table definition is really at fault
> for being dependent on what the session timezone is.
>
> Personally I'd make the ins_ts column be timestamp with time zone, but
> if you really don't want to do that, you could consider making the default
> expression be "current_timestamp AT TIME ZONE 'something'" to force the
> rotated value to be in a particular zone.
>
> regards, tom lane
>
>
>
Tom -

Attached is a doc patch that updates the documentation for postgres-fdw to
include the actual values for the 4 session variables that are set. Does
that make sense to clarify?

Thanks
-Dave
From c00f4833993899e0f78b2950358822d4b1f0011a Mon Sep 17 00:00:00 2001
From: David Rader 
Date: Wed, 1 Mar 2017 16:42:14 -0500
Subject: [PATCH] Document postgres-fdw session settings for parameters

---
 doc/src/sgml/postgres-fdw.sgml | 31 ++-
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index b31f373..eeae3cb 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -532,11 +532,32 @@
 
   
postgres_fdw likewise establishes remote session settings
-   for the parameters ,
-   , ,
-   and .  These are less likely
-   to be problematic than search_path, but can be handled
-   with function SET options if the need arises.
+   for the parameters: 
+   
+
+ 
+   is set to UTC
+ 
+
+
+ 
+   is set to ISO
+ 
+
+
+ 
+   is set to postgres
+ 
+
+
+ 
+   is set to 3 for remote
+  servers 9.0 and newer and is set to 2 for older versions
+ 
+
+   
+   These are less likely to be problematic than search_path, but 
+   can be handled with function SET options if the need arises.
   
 
   
-- 
2.5.0


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

Re: [HACKERS] 2017-03 Commitfest In Progress

[Thomas Munro]

On Thu, Mar 2, 2017 at 4:42 AM, David Steele  wrote:
> The 2017-03 commitfest is now in progress.  Here's the breakdown:
>
> Needs review: 128
> Waiting on Author: 26
> Ready for Committer: 26
> Total: 180

Not quite a record haul but close.

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


commitfests.data
Description: Binary data


plot-commitfest-graph
Description: Binary data

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

Re: [HACKERS] ANALYZE command progress checker

[Andres Freund]

On March 1, 2017 11:34:48 AM PST, David Fetter  wrote:
>I notice that that commit has no SGML component.  Should it have one?

Don't think so.

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


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

Re: [HACKERS] ANALYZE command progress checker

[David Fetter]

On Wed, Mar 01, 2017 at 10:28:23AM -0800, Andres Freund wrote:
> On 2017-03-01 10:25:49 -0800, Andres Freund wrote:
> > We now have a framework for this [1] (currently used by vacuum, but
> > extensible). The question is about presentation.  I'm fairly sure that
> > we shouldn't just add yet another framework, and I doubt that that's
> > what's proposed by Peter.
> > 
> > [1]
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c16dc1aca5e
> 
> Majority of that is actually in
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b6fb6471f6afaf649e52f38269fd8c5c60647669

If that's even vaguely usable, I'd say we should use it for this.

I notice that that commit has no SGML component.  Should it have one?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

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


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

Re: [HACKERS] timeouts in PostgresNode::psql

[Peter Eisentraut]

On 2/26/17 21:28, Craig Ringer wrote:
> Amended patch attached after a few Perl-related comments I got on
> private mail. Instead of
> 
> $exc_save !~ /^$timeout_exception.*/
> 
> I've updated to:
> 
> $exc_save !~ /^\Q$timeout_exception\E/
> 
> i.e. don't do an unnecessary wildcard match at the end, and disable
> metachar interpretation in the substituted range.
> 
> Still needs applying to pg9.6 and pg10.

committed to master and 9.6

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


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

Re: [HACKERS] Logical replication existing data copy

[Petr Jelinek]

On 28/02/17 20:36, Erik Rijkers wrote:
> This is the most frequent error that happens while doing pgbench-runs
> over logical replication: I run it continuously all day, and every few
> hours an error occurs of the kind seen below: a table (pgbench_history,
> mostly) ends up 1 row short (673466 instead of 673467).  I have the
> script wait a long time before calling it an error (because in theory it
> could still 'finish', and end successfully (although that has not
> happened yet, once the system got into this state).
> 

Yeah it's unlikely if it's just one row. It suggests there might still
be some snapbuild issue, but I don't immediately see one and I run
couple thousand repeats of the test without getting any inconsistency.
Will continue digging.

> 
> I gathered some info in this (proabably deadlocked) state in the hope
> there is something suspicious in there:
> 

Hmm that didn't really reveal much :(


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


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

Re: [HACKERS] ANALYZE command progress checker

[Andres Freund]

On 2017-03-01 10:25:49 -0800, Andres Freund wrote:
> We now have a framework for this [1] (currently used by vacuum, but
> extensible). The question is about presentation.  I'm fairly sure that
> we shouldn't just add yet another framework, and I doubt that that's
> what's proposed by Peter.
> 
> [1]
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c16dc1aca5e

Majority of that is actually in
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b6fb6471f6afaf649e52f38269fd8c5c60647669


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

Re: [HACKERS] ANALYZE command progress checker

[Andres Freund]

On 2017-03-01 10:20:41 -0800, David Fetter wrote:
> On Wed, Mar 01, 2017 at 09:45:40AM -0500, Peter Eisentraut wrote:
> > On 2/28/17 04:24, vinayak wrote:
> > > The view provides the information of analyze command progress details as 
> > > follows
> > > postgres=# \d pg_stat_progress_analyze
> > >View "pg_catalog.pg_stat_progress_analyze"
> > 
> > Hmm, do we want a separate "progress" system view for every kind of
> > command?  What if someone comes up with a progress checker for CREATE
> > INDEX, REINDEX, CLUSTER, etc.?

I don't think that'd be that bad, otherwise the naming of the fields is
complicated.  I guess the alternative (or do both?) would be to to have
a pivoted table, but that'd painful to query.  Do you have a better idea?


> Some kind of design for progress seems like a good plan.  Some ideas:
> 
> - System view(s)
> 
> This has the advantage of being shown to work at least to a PoC by
> this patch, and is similar to extant system views like
> pg_stat_activity in the sense of capturing a moment in time.
> 
> - NOTIFY
> 
> Event-driven model as opposed to a polling one.  This is
> attractive on efficiency grounds, less so on reliability ones.
> 
> - Something added to the wire protocol
> 
> More specialized, limits the information to the session where the
> command was issued
> 
> - Other things not named here

We now have a framework for this [1] (currently used by vacuum, but
extensible). The question is about presentation.  I'm fairly sure that
we shouldn't just add yet another framework, and I doubt that that's
what's proposed by Peter.

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c16dc1aca5e


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

Re: [HACKERS] ANALYZE command progress checker

[David Fetter]

On Wed, Mar 01, 2017 at 09:45:40AM -0500, Peter Eisentraut wrote:
> On 2/28/17 04:24, vinayak wrote:
> > The view provides the information of analyze command progress details as 
> > follows
> > postgres=# \d pg_stat_progress_analyze
> >View "pg_catalog.pg_stat_progress_analyze"
> 
> Hmm, do we want a separate "progress" system view for every kind of
> command?  What if someone comes up with a progress checker for CREATE
> INDEX, REINDEX, CLUSTER, etc.?

Some kind of design for progress seems like a good plan.  Some ideas:

- System view(s)

This has the advantage of being shown to work at least to a PoC by
this patch, and is similar to extant system views like
pg_stat_activity in the sense of capturing a moment in time.

- NOTIFY

Event-driven model as opposed to a polling one.  This is
attractive on efficiency grounds, less so on reliability ones.

- Something added to the wire protocol

More specialized, limits the information to the session where the
command was issued

- Other things not named here

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

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


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

Re: [HACKERS] Possible spelling fixes

[Andres Freund]

On 2017-03-01 14:40:26 -0300, Alvaro Herrera wrote:
> Josh Soref wrote:
> 
> > One thing that would be helpful is if someone could comment on:
> > https://github.com/jsoref/postgres/commit/9050882d601134ea1ba26f77ce5f1aaed75418de
> > -#undef SH_ITERTOR
> > +#undef SH_ITERATOR
> > 
> > It's unclear to me what that line is/was doing. It's possible that it
> > could be removed entirely instead of having its spelling changed.
> > If the line is trying to guard against a previous version of the code,
> > which is no longer active, then it deserves a comment.
> 
> AFAICS this is a bug.  This file can potentially be included several
> times by the same C source, and it defines SH_ITERATOR every time.  The
> second time it needs to be #undef'ed prior, which this line is supposed
> to do but fails because of the typo.

Indeed.  Fixed, thanks for noticing.

Andres


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

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

[Tels]

Hello all,

as this is my first mail to pgsql-hackers, please be gentle :)

I've looked at the patch, and as I'm not that familiar with the
pg-sourcecode, customs and so on, this isn't a review, more like food for
thought and all should be taken with a grain of salt. :)

So here are a few questions and remarks:

>+  double  limit_tuples = -1.0;

Surely the limit cannot be fractional, and must be an integer. So wouldn't
it be better the same type as say:

>+  if (root->limit_tuples >= 0.0 &&

Than you could also compare with ">= 0", not ">= 0.0".

node->ss.ps.ps_numTuples is f.i. an uint64.

Or is there a specific reason the limit must be a double?

And finally:

>+  if (node->ss.ps.ps_numTuples > 0)

>+  appendStringInfo(, " LIMIT %ld", node->ss.ps.ps_numTuples);

vs.

>+  appendStringInfo(, "%s LIMIT %lu",
>+   sql, 
>node->ss.ps.ps_numTuples);

It seems odd to have two different format strings here for the same variable.

A few comments miss "." at the end, like these:

>+   * Also, pass down the required number of tuples

>+   * Pass down the number of required tuples by the upper node

And this comment might be better "were we already called?"

>+  boolrs_started; /* are we already called? */

Hope this helps, and thank you for working on this issue.

Regards,

Tels


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Mar 1, 2017 at 12:23 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> on elif
>>  if misplaced elif
>> misplaced elif error
>>  else
>> eval expression
>>   => possible eval error
>> set new status if eval fine
>>
>
> Currently it is really:
>
>   switch (state) {
>   case NONE:
>   case ELSE_TRUE:
>   case ELSE_FALSE:
>  success = false;
>  show some error
>   default:
>   }
>   if (success) {
> success = evaluate_expression(...);
> if (success) {
>switch (state) {
>case ...:
>default:
>}
> }
>   }
>
> Which I do not find so neat. The previous one with nested switch-if-switch
> looked as bad.


That is accurate. The only positive it has is that the user only
experiences one error, and it's the first error that was encountered if
reading top-to-bottom, left to right. It is an issue of which we prioritize
- user experience or simpler code.

Now if you want to require committer opinion on this one, fine with me.


Rather than speculate on what a committer thinks of this edge case (and
making a patch for each possible theory), I'd rather just ask them what
their priorities are and which user experience they favor.

Re: [HACKERS] Patch: Optimize memory allocation in function 'bringetbitmap'

[Alvaro Herrera]

Alvaro Herrera wrote:
> Jinyu Zhang wrote:
>
> > Update the patch_brin_optimze_mem according to your comment. 
> 
> I have added this patch to the commitfest, which I've been intending to
> get in for a long time.  I'll be submitting an updated patch soon.

Here it is.  I addressed some of Tomas' comments, but not all (so this
is mostly just a rebased on Jinyu's original submission).

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 2c7963e..dc9cc2d 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -226,7 +226,8 @@ brin_page_items(PG_FUNCTION_ARGS)
if (ItemIdIsUsed(itemId))
{
dtup = brin_deform_tuple(bdesc,
-   
(BrinTuple *) PageGetItem(page, itemId));
+   
(BrinTuple *) PageGetItem(page, itemId),
+   NULL);
attno = 1;
unusedItem = false;
}
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index b22563b..04c50fe 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -182,7 +182,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
MemoryContextSwitchTo(tupcxt);
}
 
-   dtup = brin_deform_tuple(bdesc, brtup);
+   dtup = brin_deform_tuple(bdesc, brtup, NULL);
 
/*
 * Compare the key values of the new tuple to the stored index 
values;
@@ -328,6 +328,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
FmgrInfo   *consistentFn;
MemoryContext oldcxt;
MemoryContext perRangeCxt;
+   BrinMemTuple *dtup;
+   BrinTuple*btup;
+   uint32   btupInitSize;
 
opaque = (BrinOpaque *) scan->opaque;
bdesc = opaque->bo_bdesc;
@@ -348,6 +351,13 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 * key reference each of them.  We rely on zeroing fn_oid to InvalidOid.
 */
consistentFn = palloc0(sizeof(FmgrInfo) * bdesc->bd_tupdesc->natts);
+   dtup = brin_new_memtuple(bdesc);
+
+   /*
+* Estimate the approximate size of btup and allocate memory for btup.
+*/
+   btupInitSize = bdesc->bd_tupdesc->natts * 16;
+   btup = palloc(btupInitSize);
 
/*
 * Setup and use a per-range memory context, which is reset every time 
we
@@ -379,7 +389,10 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
   
scan->xs_snapshot);
if (tup)
{
-   tup = brin_copy_tuple(tup, size);
+   if(size <= btupInitSize)
+   memcpy(btup, tup, size);
+   else
+   btup = brin_copy_tuple(tup, size);
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
}
 
@@ -387,15 +400,13 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 * For page ranges with no indexed tuple, we must return the 
whole
 * range; otherwise, compare it to the scan keys.
 */
-   if (tup == NULL)
+   if (btup == NULL)
{
addrange = true;
}
else
{
-   BrinMemTuple *dtup;
-
-   dtup = brin_deform_tuple(bdesc, tup);
+   dtup = brin_deform_tuple(bdesc, btup, dtup);
if (dtup->bt_placeholder)
{
/*
@@ -1175,7 +1186,7 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple 
*b)
"brin union",

ALLOCSET_DEFAULT_SIZES);
oldcxt = MemoryContextSwitchTo(cxt);
-   db = brin_deform_tuple(bdesc, b);
+   db = brin_deform_tuple(bdesc, b, NULL);
MemoryContextSwitchTo(oldcxt);
 
for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
diff --git a/src/backend/access/brin/brin_tuple.c 
b/src/backend/access/brin/brin_tuple.c
index ec5fc56..20b0079 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -348,54 +348,69 @@ BrinMemTuple *
 brin_new_memtuple(BrinDesc *brdesc)
 {
BrinMemTuple *dtup;
-   char   *currdatum;
longbasesize;
-   int i;
 
basesize = 

Re: [HACKERS] Possible spelling fixes

[Alvaro Herrera]

Josh Soref wrote:

> One thing that would be helpful is if someone could comment on:
> https://github.com/jsoref/postgres/commit/9050882d601134ea1ba26f77ce5f1aaed75418de
> -#undef SH_ITERTOR
> +#undef SH_ITERATOR
> 
> It's unclear to me what that line is/was doing. It's possible that it
> could be removed entirely instead of having its spelling changed.
> If the line is trying to guard against a previous version of the code,
> which is no longer active, then it deserves a comment.

AFAICS this is a bug.  This file can potentially be included several
times by the same C source, and it defines SH_ITERATOR every time.  The
second time it needs to be #undef'ed prior, which this line is supposed
to do but fails because of the typo.

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


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

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

>
>
>
>1.
>
>Added explicit casts bytea=>jsonb and jsonb=>bytea (for jsonb=>bytea
>output using RETURNING bytea FORMAT JSONB and corresponding bytea=>jsonb
>input using  FORMAT JSONB).
>
>
This point has sense in Oracle, where JSON is blob. But it is little bit
obscure in PostgreSQL context.

Regards

Pavel




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

Re: [HACKERS] pg_dump, pg_dumpall and data durability

[David Steele]

On 1/22/17 11:02 PM, Michael Paquier wrote:
> On Tue, Nov 29, 2016 at 1:30 PM, Michael Paquier
>  wrote:
>> On Mon, Nov 14, 2016 at 6:07 PM, Michael Paquier
>>  wrote:
>>> On Mon, Nov 14, 2016 at 6:04 PM, Albe Laurenz  
>>> wrote:
 Michael Paquier wrote:
> Meh. I forgot docs and --help output updates.

 Looks good, except that you left the "N" option in the getopt_long
 call for pg_dumpall.  I fixed that in the attached patch.
>>>
>>> No, v5 has removed it, but it does not matter much now...
>>>
 I'll mark the patch "ready for committer".
>>>
>>> Thanks!
>>
>> Moved to CF 2017-01.
> 
> Moved to CF 2017-03 with the same status, ready for committer. Perhaps
> there is some interest in this feature? v5 of the patch still applies,
> with a couple of hunks, so v6 is attached.

This patch is in need of a committer.  Any takers?

I didn't see a lot of enthusiasm from committers on the thread so if
nobody picks it up by the end of the CF I'm going to mark the patch RWF.

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


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

Re: [HACKERS] log_autovacuum_min_duration doesn't log VACUUMs

[Jeff Janes]

On Wed, Mar 1, 2017 at 6:43 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 2/10/17 03:38, Simon Riggs wrote:
> > I guess its fairly obvious in the title, but
> > log_autovacuum_min_duration doesn't log VACUUMs only autovacuums.
> >
> > What isn't obvious is why that restruction is useful.
>
> Because there are already various tools available to log activity of
> session processes, but there are no other ways to log the activity of
> autovacuum.  Why are the existing settings not sufficient for this purpose?
>

I've pretty often been annoyed that the information provided by 'VACUUM
VERBOSE' and the information provided by log_autovacuum_min_duration are so
drastically different from each other.  It would be pretty nice to have
some way to get the same information for both operations in the same
format, although I don't know if this proposal is the best way of
accomplishing it.

Cheers,

Jeff

Re: [HACKERS] objsubid vs subobjid

[Peter Eisentraut]

On 3/1/17 09:51, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 2/22/17 19:35, Jim Nasby wrote:
>>> pg_get_object_address() currently returns a field called subobjid, while 
>>> pg_depend calls that objsubid. I'm guessing that wasn't on purpose 
>>> (especially because internally the function uses objsubid), and it'd be 
>>> nice to fix it.
>>
>> I'm in favor of changing it, but it could theoretically break someone's
>> code.
> 
> Yes, it was an oversight.  +1 for changing.

OK done.

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


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Corey,


on elif
 if misplaced elif
misplaced elif error
 else
eval expression
  => possible eval error
set new status if eval fine


Currently it is really:

  switch (state) {
  case NONE:
  case ELSE_TRUE:
  case ELSE_FALSE:
 success = false;
 show some error
  default:
  }
  if (success) {
success = evaluate_expression(...);
if (success) {
   switch (state) {
   case ...:
   default:
   }
}
  }

Which I do not find so neat. The previous one with nested switch-if-switch 
looked as bad.



The issue at hand being the benefit to the user vs code complexity.


Hmmm.

One of my point is that I do not really see the user benefit... for me the 
issue is to have no user benefit and code complexity.


The case we are discussing is for the user who decides to write code with 
*two* errors on the same line:


  \if good-condition
  \else
  \elif bad-condition
  \endif

with an added complexity to show the elif bad position error first. Why 
should we care so much for such a special case?


Maybe an alternative could be to write simpler code anyway, somehow like 
it was before:


  // on "elif"
  switch (peek(state)) {
  case NONE:   error;
  case ELSE_TRUE:  error;
  case ELSE_FALSE: error;
  case IGNORED:break;
  case TRUE:   poke IGNORED;
  case FALSE:
   success = evaluate(_true)
   if (!success)
 error;
   else if (is_true)
   poke TRUE
  default: error;
  }

The only difference is that the evaluation is not done when it is not 
needed (what a draw back) but ISTM that it is significantly easier to 
understand and maintain.


Now if you want to require committer opinion on this one, fine with me.

--
Fabien.


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

Re: [HACKERS] Restricting maximum keep segments by repslots

[Peter Eisentraut]

On 2/27/17 23:27, Petr Jelinek wrote:
>>> WARNING:  restart LSN of replication slots is ignored by checkpoint
>>> DETAIL:  Some replication slots lose required WAL segnents to continue.
> However this is dangerous as logical replication slot does not consider
> it error when too old LSN is requested so we'd continue replication,
> hiding data loss.

In general, we would need a much more evident and strict way to discover
when this condition is hit.  Like a "full" column in
pg_stat_replication_slot, and refusing connections to the slot until it
is cleared.

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


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

Re: [HACKERS] Restricting maximum keep segments by repslots

[Peter Eisentraut]

On 2/27/17 22:27, Kyotaro HORIGUCHI wrote:
> This patch adds a GUC to put a limit to the number of segments
> that replication slots can keep.

Please measure it in size, not in number of segments.

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


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

Re: [HACKERS] Possible spelling fixes

[Josh Soref]

Peter Eisentraut wrote:
> Yes, some of that was committed, and some comments were offered.  If
> there is more to do, please send a rebased patch set.

Conflicting comments were offered. And Heikki requested I send along
the remainder. Which I did. Only one of those patches would have been
impacted by the conflicting comments.

The patches in this thread still applied today:
spelling: comments -- conflicting comments about NUL/NULL
spelling: strings -- no comments / applied cleanly
spelling: variables -- no comments / applied cleanly
spelling: misc -- no comments / applied cleanly
spelling: api -- no comments until today / applied cleanly, may end up
being dropped

I want to thank Heikki for the initial acceptance and Alvaro and David
for their additional comments.

I'm not going to send a new submission before tonight.

If anyone else wants to make comments before I resubmit, I welcome them...

For reference, my current work queue is here:
https://github.com/postgres/postgres/compare/master...jsoref:spelling-words?expand=1

The rebased version of the patches that were submitted but ignored are here:
https://github.com/postgres/postgres/compare/master...jsoref:spelling?expand=1
I haven't updated them to reflect my work queue, as selecting things
into those bundles is a not particularly pleasant, and thus a step I
want to do as few times as possible.

One thing that would be helpful is if someone could comment on:
https://github.com/jsoref/postgres/commit/9050882d601134ea1ba26f77ce5f1aaed75418de
-#undef SH_ITERTOR
+#undef SH_ITERATOR

It's unclear to me what that line is/was doing. It's possible that it
could be removed entirely instead of having its spelling changed.
If the line is trying to guard against a previous version of the code,
which is no longer active, then it deserves a comment.


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

Re: [HACKERS] many copies of atooid() and oid_cmp()

[Peter Eisentraut]

On 1/12/17 09:36, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 1/11/17 11:25 PM, Tom Lane wrote:
>>> +1 for the concept, but I'm a bit worried about putting atooid() in
>>> postgres_ext.h.  That's going to impose on the namespace of libpq-using
>>> applications, for instance.  A more conservative answer would be to
>>> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
>>> so I do see the consistency of adding this there.  Hard choice.
> 
>> How about two copies: one in postgres_fe.h and one in postgres.h?
> 
> That seems uglier than either of the other choices.
> 
> I don't personally have a huge problem with adding atooid in
> postgres_ext.h, but I thought I'd better flag the potential issue
> to see if anyone else thinks it's a big problem.

committed as is then

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


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

Re: [HACKERS] Skip all-visible pages during second HeapScan of CIC

[Masahiko Sawada]

On Tue, Feb 28, 2017 at 10:42 PM, Pavan Deolasee
 wrote:
> Hello All,
>
> During the second heap scan of CREATE INDEX CONCURRENTLY, we're only
> interested in the tuples which were inserted after the first scan was
> started. All such tuples can only exists in pages which have their VM bit
> unset. So I propose the attached patch which consults VM during second scan
> and skip all-visible pages. We do the same trick of skipping pages only if
> certain threshold of pages can be skipped to ensure OS's read-ahead is not
> disturbed.

Great.

>
> The patch obviously shows significant reduction of time for building index
> concurrently for very large tables, which are not being updated frequently
> and which was vacuumed recently (so that VM bits are set). I can post
> performance numbers if there is interest.

Please share it. I'm curious.

> For tables that are being updated
> heavily, the threshold skipping was indeed useful and without that we saw a
> slight regression.
>
> Since VM bits are only set during VACUUM which conflicts with CIC on the
> relation lock, I don't see any risk of incorrectly skipping pages that the
> second scan should have scanned.

Agreed.

And the patch looks good to me.

Regards,

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


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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Fabien COELHO]

Hello Peter,


I wrote a few lines of perl to move replaceable out of option and did some
manual editing is special cases, the resulting simple 359 changes is
attached.


If the stylesheet produces unpleasant output, then the stylesheet should
be changed.


Sure.

I'm not sure whether it is a stylesheet issue: it is the stylesheet as 
interpreted by chrome... all is fine with firefox. Whether the bug is in 
chrome or the stylesheet or elsewhere is well beyond my HTML/CSS skills, 
but I can ask around.



The current markup looks fine (to me) with the minimal default/non-web
stylesheet, so the issue is somewhere else.


Attached how "Google Chrome 56.0.2924.87 (Official Build) (64-bit)" on my 
ubuntu laptop shows the options in of the psql dev doc using the online 
css.


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

Re: [HACKERS] Possible spelling fixes

[Josh Soref]

I'll include it.


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

Re: [HACKERS] Possible spelling fixes

[Josh Soref]

I understood that they were git commits. I could have excluded the
file but opted not to in case people were willing to take a small
drift -- the SHAs are what someone needs to review the commit, and
personally, I'd rather read something without typos than with --
especially in a summary. But, I'll tentatively exclude the lines
containing SHAs.

do you want:
@@ -3116,7 +3116,7 @@ 2016-02-17 [a5c43b886] Add new system vi


 This view exposes the same information available from
-the pg_config comand-line utility,
+the pg_config command-line utility,
 namely assorted compile-time configuration information for
 PostgreSQL.


or should I exclude the file entirely?


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

Re: [HACKERS] Possible spelling fixes

[Josh Soref]

I can easily drop the shutdown* items;

The reason for rowMarks is consistency with lr_arowMarks.

I'll tentatively exclude that from any resubmission I make tonight...


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Mar 1, 2017 at 3:07 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> It doesn't strike me as much cleaner, but it's no worse, either.
>>
>
> Hmmm.
>
> The "if (x) { x = ... ; if (x) {" does not help much to improve
> readability and understandability...
>
> My 0.02€ about v19:
>
> If there are two errors, I do not care which one is shown, both will have
> to be fixed anyway in the end... So I would suggest to choose the simplest
> possible implementation:
>
>   on elif:
> always eval expression
>   => possible eval error
> switch
>   => including detecting misplaced elif errors
>
> If the second error must absolutely be shown in all cases, then add a
> second misplaced elif detection in the eval expression failure branch:
>
>   on elif
> always eval
> if (eval failed)
>   also checked for misplaced (hey user, you have 2 errors in fact...)
>   bye bye...
> // else eval was fine
> switch
>   including misplaced elif detection
>
> If the committer is angry at these simple approach, then revert to the
> strange looking and hard to understand switch-if-switch solution (~ v18, or
> some simplified? v19), but I do not think the be weak benefit is worth the
> code complexity.
>
> --
> Fabien.


Just to make things easy for the committer, the existing code only shows
the user one error:

on elif
  if misplaced elif
 misplaced elif error
  else
 eval expression
   => possible eval error
 set new status if eval fine


The issue at hand being the benefit to the user vs code complexity.

So, shall we send this off to the committers and let them decide?

Re: [HACKERS] Time to drop old-style (V0) functions?

[Andres Freund]

On 2017-03-01 11:18:34 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-12-08 13:34:41 -0800, Andres Freund wrote:
> >> Hi,
> >> 
> >> I'm wondering if it's not time for $subject:
> >> - V0 causes confusion / weird crashes when PG_FUNCTION_INFO_V1 was
> >> forgotten
> >> - They have us keep weird hacks around just for the sake of testing V0
> >> - they actually cost performance, because we have to zero initialize 
> >> Datums, even if
> >> the corresponding isnull marker is set.
> >> - they allow to call arbitrary functions pretty easily
> >> 
> >> I don't see any reason to keep them around. If seriously doubt anybody
> >> is using them seriously in anything but error.
> 
> I find these arguments pretty weak.  In particular I don't buy your claim
> that we could stop zero-initializing Datums, because I think we'd just
> start getting valgrind complaints if we did.

I tink we've litigated that in a side-thread - I'm not planning to change
any policy around this. Perhaps I shouldn't have quoted the original
start of the thread...

At this point I primarily am concerned about
a) the way they're confusing for authors, by causing spurious crashes
b) at some point not too far away in the future I want to introduce a
   faster interface, and I don't want unnecessarily many around.

Greetings,

Andres Freund


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

Re: [HACKERS] BRIN cost estimate

[Alvaro Herrera]

Alvaro Herrera wrote:

> I have added this patch to the commitfest, which I've been intending to
> get in for a long time.  I'll be submitting an updated patch, if needed.

Here is Emre's patch rebased to current sources.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 8b05e8f..f462436 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -100,8 +100,10 @@
 #include 
 #include 
 
+#include "access/brin.h"
 #include "access/gin.h"
 #include "access/htup_details.h"
+#include "access/reloptions.h"
 #include "access/sysattr.h"
 #include "catalog/index.h"
 #include "catalog/pg_am.h"
@@ -7541,14 +7543,21 @@ brincostestimate(PlannerInfo *root, IndexPath *path, 
double loop_count,
 {
IndexOptInfo *index = path->indexinfo;
List   *indexQuals = path->indexquals;
-   List   *indexOrderBys = path->indexorderbys;
double  numPages = index->pages;
double  numTuples = index->tuples;
+   RelOptInfo *baserel = index->rel;
List   *qinfos;
Costspc_seq_page_cost;
Costspc_random_page_cost;
double  qual_op_cost;
double  qual_arg_cost;
+   double  qualSelectivity;
+   double  blockProportion;
+   double  numBlocks;
+   double  blockSelectivity;
+   double  selec;
+   RelationindexRel;
+   VariableStatData vardata;
 
/* Do preliminary analysis of indexquals */
qinfos = deconstruct_indexquals(path);
@@ -7561,7 +7570,8 @@ brincostestimate(PlannerInfo *root, IndexPath *path, 
double loop_count,
/*
 * BRIN indexes are always read in full; use that as startup cost.
 *
-* XXX maybe only include revmap pages here?
+* XXX We should consider the revmap at seqpage cost, and regular pages 
at
+* random page cost.
 */
*indexStartupCost = spc_seq_page_cost * numPages * loop_count;
 
@@ -7572,24 +7582,190 @@ brincostestimate(PlannerInfo *root, IndexPath *path, 
double loop_count,
 */
*indexTotalCost = spc_random_page_cost * numPages * loop_count;
 
-   *indexSelectivity =
-   clauselist_selectivity(root, indexQuals,
-  
path->indexinfo->rel->relid,
-  JOIN_INNER, NULL);
-   *indexCorrelation = 1;
+   /*
+* Compute index correlation.
+*
+* Because we can use all index quals equally when scanning, we can use
+* the largest correlation (in absolute value) among columns used by the
+* query. Start at zero, the worst possible case.
+*/
+   *indexCorrelation = 0;
+
+   {
+   RangeTblEntry *rte = planner_rt_fetch(index->rel->relid, root);
+   Oid relid;
+   ListCell   *cell;
+
+   Assert(rte->rtekind == RTE_RELATION);
+   relid = rte->relid;
+   Assert(relid != InvalidOid);
+
+   foreach(cell, qinfos)
+   {
+   IndexQualInfo *qinfo = (IndexQualInfo *) lfirst(cell);
+   AttrNumber  colnum = 
index->indexkeys[qinfo->indexcol];
+
+   if (colnum != 0)
+   {
+   /* Simple variable -- look to stats for the 
underlying table */
+   if (get_relation_stats_hook &&
+   (*get_relation_stats_hook) (root, rte, 
colnum, ))
+   {
+   /*
+* The hook took control of acquiring a 
stats tuple.  If
+* it did supply a tuple, it'd better 
have supplied a
+* freefunc.
+*/
+   if 
(HeapTupleIsValid(vardata.statsTuple) &&
+   !vardata.freefunc)
+   elog(ERROR, "no function 
provided to release variable stats with");
+   }
+   else
+   {
+   vardata.statsTuple = 
SearchSysCache3(STATRELATTINH,
+   
 ObjectIdGetDatum(relid),
+   
   Int16GetDatum(colnum),
+   /* XXX no inh */
+   

Re: [HACKERS] objsubid vs subobjid

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 2/22/17 19:35, Jim Nasby wrote:
> > pg_get_object_address() currently returns a field called subobjid, while 
> > pg_depend calls that objsubid. I'm guessing that wasn't on purpose 
> > (especially because internally the function uses objsubid), and it'd be 
> > nice to fix it.
> 
> I'm in favor of changing it, but it could theoretically break someone's
> code.

Yes, it was an oversight.  +1 for changing.

> I don't know what the practical use for these functions is.

This was originally written for BDR use in DDL replication.  Partly the
interfaces exist for testing purposes (to make sure that object
addresses can roundtrip between internal OID numerical representation
and set of names); what BDR uses is the path that goes via event
triggers (pg_event_trigger_ddl_commands and pg_event_trigger_dropped_objects).
I didn't find any use of the name "subobjid" anywhere in BDR.

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


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

Re: [HACKERS] Possible spelling fixes

[Alvaro Herrera]

Josh Soref wrote:
> 

>  

Re: [HACKERS] Possible spelling fixes

[Alvaro Herrera]

Josh Soref wrote:

> - else if (ControlFile->state == DB_SHUTDOWNING)
> + else if (ControlFile->state == DB_SHUTTINGDOWN)

SHUTDOWNING and SHUTDOWNED are typos first introduced by hacker emeritus
Vadim Mikheev in 1999 together with WAL, commit 47937403676d.  It goes
to show that not every PG hacker is a learned English speaker.  I see no
reason to change the spelling.  

> - List   *epq_arowmarks;
> + List   *epq_arowMarks;

I'd not change this one either.  Of the large set of words run together
in our source, this is one of the easiest ones to read.

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


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

Re: [HACKERS] WAL logging problem in 9.4.3?

[David Steele]

On 1/30/17 11:33 PM, Michael Paquier wrote:
> On Fri, Dec 2, 2016 at 1:39 PM, Haribabu Kommi  
> wrote:
>> The latest proposed patch still having problems.
>> Closed in 2016-11 commitfest with "moved to next CF" status because of a bug
>> fix patch.
>> Please feel free to update the status once you submit the updated patch.
> And moved to CF 2017-03...

Are there any plans to post a new patch?  This thread is now 18 months
old and it would be good to get a resolution in this CF.

Thanks,

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



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

Re: [HACKERS] perlcritic

[Dagfinn Ilmari Mannsåker]

Hi Peter,

Peter Eisentraut  writes:

> I posted this about 18 months ago but then ran out of steam. [ ] Here
> is an updated patch.  The testing instructions below still apply.
> Especially welcome would be ideas on how to address some of the places
> I have marked with ## no critic.

Attached is a patch on top of yours that addresses all the ## no critic
annotations except RequireFilenameMatchesPackage, which can't be fixed
without more drastic reworking of the plperl build process.

Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and
--enable-tap-tests followed by make check-world, and running pgindent
--build.

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

>From cdf3ca19cbbf03111243f9b39eb6f402f25b4502 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 1 Mar 2017 15:32:45 +
Subject: [PATCH] Fix most perlcritic exceptions

The RequireFilenameMatchesPackage ones would require reworking the
plperl build process more drastically.
---
 src/pl/plperl/plc_perlboot.pl | 9 +++--
 src/tools/msvc/gendef.pl  | 2 +-
 src/tools/pgindent/pgindent   | 6 +++---
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
index 292c9101c9..b4212f5ab2 100644
--- a/src/pl/plperl/plc_perlboot.pl
+++ b/src/pl/plperl/plc_perlboot.pl
@@ -81,18 +81,15 @@ sub ::encode_array_constructor
 		} sort keys %$imports;
 		$BEGIN &&= "BEGIN { $BEGIN }";
 
-		return qq[ package main; sub { $BEGIN $prolog $src } ];
+		# default no strict and no warnings
+		return qq[ package main; sub { no strict; no warnings; $BEGIN $prolog $src } ];
 	}
 
 	sub mkfunc
 	{
-		## no critic (ProhibitNoStrict, ProhibitStringyEval);
-		no strict;  # default to no strict for the eval
-		no warnings;# default to no warnings for the eval
-		my $ret = eval(mkfuncsrc(@_));
+		my $ret = eval(mkfuncsrc(@_)); ## no critic (ProhibitStringyEval);
 		$@ =~ s/\(eval \d+\) //g if $@;
 		return $ret;
-		## use critic
 	}
 
 	1;
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index 64227c2dce..e2653f11d8 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -174,7 +174,7 @@ sub usage
 
 my %def = ();
 
-while (<$ARGV[0]/*.obj>)  ## no critic (RequireGlobFunction);
+while (glob($ARGV[0]/*.obj))
 {
 	my $objfile = $_;
 	my $symfile = $objfile;
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index a6b24b5348..51d6a28953 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -159,8 +159,7 @@ sub process_exclude
 		while (my $line = <$eh>)
 		{
 			chomp $line;
-			my $rgx;
-			eval " \$rgx = qr!$line!;";  ## no critic (ProhibitStringyEval);
+			my $rgx = eval { qr!$line! };
 			@files = grep { $_ !~ /$rgx/ } @files if $rgx;
 		}
 		close($eh);
@@ -435,7 +434,8 @@ sub diff
 
 sub run_build
 {
-	eval "use LWP::Simple;";  ## no critic (ProhibitStringyEval);
+	require LWP::Simple;
+	LWP::Simple->import(qw(getstore is_success));
 
 	my $code_base = shift || '.';
 	my $save_dir = getcwd();
-- 
2.11.0


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

Re: [HACKERS] Time to drop old-style (V0) functions?

[Tom Lane]

Andres Freund  writes:
> On 2016-12-08 13:34:41 -0800, Andres Freund wrote:
>> Hi,
>> 
>> I'm wondering if it's not time for $subject:
>> - V0 causes confusion / weird crashes when PG_FUNCTION_INFO_V1 was
>> forgotten
>> - They have us keep weird hacks around just for the sake of testing V0
>> - they actually cost performance, because we have to zero initialize Datums, 
>> even if
>> the corresponding isnull marker is set.
>> - they allow to call arbitrary functions pretty easily
>> 
>> I don't see any reason to keep them around. If seriously doubt anybody
>> is using them seriously in anything but error.

I find these arguments pretty weak.  In particular I don't buy your claim
that we could stop zero-initializing Datums, because I think we'd just
start getting valgrind complaints if we did.  It's too common to copy both
a Datum and its isnull flag without any particular inquiry into whether
the datum is null.

regards, tom lane


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

Re: [HACKERS] Restricting maximum keep segments by repslots

[Andres Freund]

Hi,

On 2017-02-28 12:42:32 +0900, Michael Paquier wrote:
> Please no. Replication slots are designed the current way because we
> don't want to have to use something like wal_keep_segments as it is a
> wart, and this applies as well for replication slots in my opinion.

I think a per-slot option to limit the amount of retention would make
sense.

- Andres


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

Re: [HACKERS] Statement-level rollback

[Tom Lane]

Peter Eisentraut  writes:
> On 2/28/17 08:17, Tom Lane wrote:
>> I do not really see how this would ever get past the compatibility
>> problems that forced us to give up on server-side autocommit years ago.

> I think it's different because it's not a global setting, it's only a
> behavior you select explicitly when you start a transaction block.

Yeah, that's the same it-won't-affect-you-if-you-don't-use-it argument
that we heard for server-side autocommit-off.  I don't buy it.
I can think of two reasons even without any caffeine:

1. The argument for this is mostly, if not entirely, "application
compatibility".  But it won't succeed at providing that if every
BEGIN has to be spelled differently than it would be on other DBMSes.
Therefore there is going to be enormous pressure to allow enabling
the feature through a GUC, or some other environment-level way,
and as soon as we do that we've lost.

2. The proposed feature would affect the internal operation of PL
functions, so that those would need to become bulletproof against
being invoked in either operating environment.  Likewise, all sorts
of intermediate tools like connection poolers would no doubt be broken
if they don't know about this and support both modes.  (We would have
to start by fixing postgres_fdw and dblink, for instance.)

In short, you can't make fundamental changes in transactional behavior
without enormous breakage.  That was the lesson we learned from the
autocommit fiasco and I do not believe that it's inapplicable here.

regards, tom lane


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

Re: [HACKERS] some dblink refactoring

[Peter Eisentraut]

On 2/28/17 22:22, Corey Huinker wrote:
> Any chance we can make get_connect_string() a core function or at least
> externally accessible?

[get_connect_string() gets the connection string for a foreign server]

The connection string for a foreign server depends on the nature of the
foreign server.  dblink can assume it's a PostgreSQL server, but it's
not clear how to generalize that.

Some kind of node or connection registry (i.e., "native" servers) might
be a better feature to think about here.

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


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

Re: [HACKERS] Possible spelling fixes

[Peter Eisentraut]

On 3/1/17 09:12, Josh Soref wrote:
> On Mar 1, 2017 9:06 AM, "Peter Eisentraut"
>  > wrote:
> 
> On 2/6/17 06:03, Heikki Linnakangas wrote:
> > Ah, yes please. Post them over and I'll have a look at those as well.
> 
> This thread is in the commit fest, but I think there is no current
> patch.
> 
> 
> I sent email on the 6th with a number of patches as attachments... 

Yes, some of that was committed, and some comments were offered.  If
there is more to do, please send a rebased patch set.

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


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

Re: [HACKERS] New Committer - Andrew Gierth

[Joe Conway]

On 02/28/2017 10:22 AM, Stephen Frost wrote:
> Greetings!
> 
> The PostgreSQL committers would like to welcome Andrew Gierth as a new
> committer for the PostgreSQL project.
> 
> Andrew - welcome!

Congratulations!

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Should we cacheline align PGXACT?

[Ashutosh Sharma]

On Wed, Mar 1, 2017 at 5:29 PM, Simon Riggs  wrote:
>
> On 1 March 2017 at 04:50, Ashutosh Sharma  wrote:
>>
>> On Tue, Feb 28, 2017 at 11:44 PM, Simon Riggs  wrote:
>>>
>>> On 28 February 2017 at 11:34, Ashutosh Sharma  wrote:
>>>

 So, Here are the pgbench results I got with 
 'reduce_pgxact_access_AtEOXact.v2.patch' on a read-write workload.
>>>
>>>
>>> Thanks for performing a test.
>>>
>>> I see a low yet noticeable performance gain across the board on that 
>>> workload.
>>>
>>> That is quite surprising to see a gain on that workload. The main workload 
>>> we have been discussing was the full read-only test (-S). For that case the 
>>> effect should be much more noticeable based upon Andres' earlier comments.
>>>
>>> Would it be possible to re-run the test using only the -S workload? Thanks 
>>> very much.
>>
>>
>> Okay, I already had the results for read-oly workload but just forgot to 
>> share it along with the results for read-write test. Here are the results 
>> for read-only
>> test,
>
>
> Thanks for conducting those comparisons.
>
> So we have variable and sometimes significant gains, with no regressions.
>
> By the shape of the results this helps in different places to the alignment 
> patch. Do we have evidence to commit both?

Well, We have seen some regression in read-write test with pgxact
alignment patch - [1]. I may need to run the test with both the
patches to see the combined effect on performance.

[1] - 
https://www.postgresql.org/message-id/CAE9k0Pk%2BrCuNY%2B7O5XwVXHPuki9t8%3DM7jr4kevxw-hdkpFhS2A%40mail.gmail.com


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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Peter Eisentraut]

On 2/19/17 12:34, Fabien COELHO wrote:
> I wrote a few lines of perl to move replaceable out of option and did some 
> manual editing is special cases, the resulting simple 359 changes is 
> attached.

If the stylesheet produces unpleasant output, then the stylesheet should
be changed.

The current markup looks fine (to me) with the minimal default/non-web
stylesheet, so the issue is somewhere else.

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


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

[HACKERS] 2017-03 Commitfest In Progress

[David Steele]

The 2017-03 commitfest is now in progress.  Here's the breakdown:

Needs review: 128
Waiting on Author: 26
Ready for Committer: 26
Total: 180

If you are an author and have a patch in the "Waiting for Author" state
please update it as soon as you can.  Otherwise, jump in and start
reviewing!

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


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

Re: [HACKERS] Possible spelling fixes

[David Rowley]

On 6 February 2017 at 15:50, Josh Soref  wrote:
> It's now split more or less to your suggestion:
> https://github.com/jsoref/postgres/commits/spelling

- * Note that this algrithm is know to not be very effective (O(N^2))
+ * Note that this algorithm is know to not be very effective (O(N^2))

know should be known. Perhaps you can include if you have a followup patch.


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


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

Re: [HACKERS] Statement-level rollback

[Peter Eisentraut]

On 2/28/17 02:39, Tsunakawa, Takayuki wrote:
> I'd like to propose statement-level rollback feature.  To repeat myself, this 
> is requested for users to migrate from other DBMSs to PostgreSQL.  They 
> expect that a failure of one SQL statement should not abort the entire 
> transaction and their apps (client programs and stored procedures) can 
> continue the transaction with a different SQL statement.

Can you provide some references on how other systems provide this feature?

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


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

Re: [HACKERS] Statement-level rollback

[Peter Eisentraut]

On 2/28/17 08:17, Tom Lane wrote:
> I do not really see how this would ever get past the compatibility
> problems that forced us to give up on server-side autocommit years ago.

I think it's different because it's not a global setting, it's only a
behavior you select explicitly when you start a transaction block.

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


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

Re: [HACKERS] Time to drop old-style (V0) functions?

[Peter Eisentraut]

On 3/1/17 02:22, Andres Freund wrote:
> One unaddressed question in those patches is what we do with
> src/backend/utils/fmgr/README - I'm not quite sure what its purpose is,
> in its current state.  If we want to keep it, we'd probably have to
> pretty aggressively revise it?

Some of the information in there, such as the use of the FmgrInfo and
FunctionCallInfoData structs, doesn't seem to appear anywhere else in
that amount of detail, so it would be a loss to just delete the file, I
think.  Perhaps just lightly editing out the "I propose to do this" tone
would work.

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


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

Re: [HACKERS] [PATCH] Use $ parameters as replacement characters for pg_stat_statements

[Peter Eisentraut]

On 2/28/17 20:01, Lukas Fittl wrote:
> Currently pg_stat_statements replaces constant values with ? characters.
> I've seen this be a problem on multiple occasions, in particular since
> it conflicts with the use of ? as an operator.
> 
> I'd like to propose changing the replacement character from ? to instead
> be a parameter (like $1).

Hmm, I think this could confuse people into thinking that the queries
displayed were in fact prepared queries.

Maybe we could gather some more ideas.

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


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

Re: [HACKERS] ANALYZE command progress checker

[Peter Eisentraut]

On 2/28/17 04:24, vinayak wrote:
> The view provides the information of analyze command progress details as 
> follows
> postgres=# \d pg_stat_progress_analyze
>View "pg_catalog.pg_stat_progress_analyze"

Hmm, do we want a separate "progress" system view for every kind of
command?  What if someone comes up with a progress checker for CREATE
INDEX, REINDEX, CLUSTER, etc.?

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


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

Re: [HACKERS] log_autovacuum_min_duration doesn't log VACUUMs

[Peter Eisentraut]

On 2/10/17 03:38, Simon Riggs wrote:
> I guess its fairly obvious in the title, but
> log_autovacuum_min_duration doesn't log VACUUMs only autovacuums.
> 
> What isn't obvious is why that restruction is useful.

Because there are already various tools available to log activity of
session processes, but there are no other ways to log the activity of
autovacuum.  Why are the existing settings not sufficient for this purpose?

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


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

Re: [HACKERS] [POC] hash partitioning

[Aleksander Alekseev]

> We can achieve desired result through creating a separate partitioned table
> and making the DETACH/ATTACH manipulation, though. But IMO it's not flexible
> case.

I think it would be great to allow end user to decide. If user is
not interested in subpartitions he or she can use syntax like 'CREATE
TABLE ... PARTITION BY HAHS(i) PARTITIONS 3 CREATE AUTOMATICALLY;' or
maybe a build-in procedure for this. Otherwise there is also
ATTACH/DETACH syntax available.

Anyway all of this is something that could be discussed infinitely and
not necessarily should be included in this concrete patch. We could
probably agree that 3 or 4 separately discussed, reviewed and tested
patches are better than one huge patch that will be moved to the next
commitfest because of disagreements regarding a syntax.

On Wed, Mar 01, 2017 at 05:10:34PM +0300, Maksim Milyutin wrote:
> On 01.03.2017 05:14, Amit Langote wrote:
> > Nagata-san,
> > 
> > > A partition table can be create as bellow;
> > > 
> > >  CREATE TABLE h1 PARTITION OF h;
> > >  CREATE TABLE h2 PARTITION OF h;
> > >  CREATE TABLE h3 PARTITION OF h;
> > > 
> > > FOR VALUES clause cannot be used, and the partition bound is
> > > calclulated automatically as partition index of single integer value.
> > > 
> > > When trying create partitions more than the number specified
> > > by PARTITIONS, it gets an error.
> > > 
> > > postgres=# create table h4 partition of h;
> > > ERROR:  cannot create hash partition more than 3 for h
> > 
> > Instead of having to create each partition individually, wouldn't it be
> > better if the following command
> > 
> > CREATE TABLE h (i int) PARTITION BY HASH (i) PARTITIONS 3;
> > 
> > created the partitions *automatically*?
> 
> It's a good idea but in this case we can't create hash-partition that is
> also partitioned table, and as a consequence we are unable to create
> subpartitions. My understanding is that the table can be partitioned only
> using CREATE TABLE statement, not ALTER TABLE. For this reason the new
> created partitions are only regular tables.
> 
> We can achieve desired result through creating a separate partitioned table
> and making the DETACH/ATTACH manipulation, though. But IMO it's not flexible
> case.
> 
> It would be a good thing if a regular table could be partitioned through
> separate command. Then your idea would not be restrictive.
> 
> 
> -- 
> Maksim Milyutin
> Postgres Professional: http://www.postgrespro.com
> Russian Postgres Company
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature