Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values

[Craig Ringer]

On 22 July 2016 at 13:24, Craig Ringer  wrote:

>
> On 22 July 2016 at 07:02, Tom Lane  wrote:
>
>> In
>>
>> https://www.postgresql.org/message-id/tencent_5c738eca65bad6861aa43...@qq.com
>> it was pointed out that you could get an intra-function-call memory leak
>> from something like
>>
>> LOOP
>>   BEGIN
>> EXECUTE 'bogus command';
>>   EXCEPTION WHEN OTHERS THEN
>>   END;
>> END LOOP;
>
>
> ...
>
>
>>
>> Another answer is to invent a third per-function memory context intended
>> to hold statement-lifespan variables.
>>
>>
> I've wanted this in the past and been surprised we don't have it, so +1
> from me.
>
> I don't think a few MemoryContextAlloc's are too ugly.
>
> I suggest that in cassert builds, or maybe just CACHE_CLOBBER_ALWAYS, the
> context is reset after each call even when not cleaning up after an error.
> That'll help catch mistakes and leaks.
>


That is, after each statement.


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

Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values

[Craig Ringer]

On 22 July 2016 at 07:02, Tom Lane  wrote:

> In
>
> https://www.postgresql.org/message-id/tencent_5c738eca65bad6861aa43...@qq.com
> it was pointed out that you could get an intra-function-call memory leak
> from something like
>
> LOOP
>   BEGIN
> EXECUTE 'bogus command';
>   EXCEPTION WHEN OTHERS THEN
>   END;
> END LOOP;


...


>
> Another answer is to invent a third per-function memory context intended
> to hold statement-lifespan variables.
>
>
I've wanted this in the past and been surprised we don't have it, so +1
from me.

I don't think a few MemoryContextAlloc's are too ugly.

I suggest that in cassert builds, or maybe just CACHE_CLOBBER_ALWAYS, the
context is reset after each call even when not cleaning up after an error.
That'll help catch mistakes and leaks.

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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Craig Ringer]

On 22 July 2016 at 01:31, Tom Lane  wrote:

> David Steele  writes:
> > On 7/21/16 12:19 PM, Robert Haas wrote:
> >> On Wed, Jul 20, 2016 at 7:42 PM, Michael Paquier
> >>  wrote:
>  People have, in the past, expressed concerns about linking in
>  pgcrypto.  Apparently, in some countries, it's a legal problem.
>
> >>> Do you have any references? I don't see that as a problem.
>
> >> I don't have a link to previous discussion handy, but I definitely
> >> recall that it's been discussed.  I don't think that would mean that
> >> libpgcrypto couldn't depend on libpgcommon, but the reverse direction
> >> would make libpgcrypto essentially mandatory which I don't think is a
> >> direction we want to go for both technical and legal reasons.
>
> > I searched a few different ways and finally came up with this post from
> Tom:
> > https://www.postgresql.org/message-id/11392.1389991...@sss.pgh.pa.us
> > It's the only thing I could find, but thought it might jog something
> > loose for somebody else.
>
> Way back when, like fifteen years ago, there absolutely were US export
> control restrictions on software containing crypto.  I believe the US has
> figured out that that was silly, but I'm not sure everyplace else has.
>

Australia has recently enacted laws that are reminiscent of the US's
defunct crypto export control laws, but they add penalties for *teaching*
encryption too. Yup, you can be charged for talking about it. Of course
they'll only actually USE those new powers to Stop The Terrorist Threat,
they promise...

http://www.defence.gov.au/deco/DTC.asp

Unless recently amended, they even failed to exclude academic institutions.
I haven't been following it closely because, frankly, it's too ridiculous
to pay much attention to, and I don't work directly with crypto anyway. But
it's far from the only such colossally ignorant and idiotic law floating
around.

Despite the technical frustrations involved, we should keep crypto
implementations in a separate library. I agree with Tom that one-way hashes
are not a practical concern, even if the laws are probably written too
poorly to draw a distinction.

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

Re: [HACKERS] Constraint merge and not valid status

[Amit Langote]

On 2016/07/22 0:38, Robert Haas wrote:
> On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
>  wrote:
>> Consider a scenario where one adds a *valid* constraint on a inheritance
>> parent which is then merged with a child table's *not valid* constraint
>> during inheritance recursion.  If merged, the constraint is not checked
>> for the child data even though it may have some.  Is that an oversight?
> 
> Seems like it.  I'd recommend we just error out in that case and tell
> the user that they should validate the child's constraint first.

Agreed.

Patch attached.  In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging.  Also added some tests for
both cases.

Thanks,
Amit
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e997b57..f6f0691 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -105,7 +105,7 @@ static void StoreConstraints(Relation rel, List *cooked_constraints,
  bool is_internal);
 static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
 			bool allow_merge, bool is_local,
-			bool is_no_inherit);
+			bool is_no_inherit, bool is_not_valid);
 static void SetRelationNumChecks(Relation rel, int numchecks);
 static Node *cookConstraint(ParseState *pstate,
 			   Node *raw_constraint,
@@ -2301,7 +2301,8 @@ AddRelationNewConstraints(Relation rel,
 			 */
 			if (MergeWithExistingConstraint(rel, ccname, expr,
 			allow_merge, is_local,
-			cdef->is_no_inherit))
+			cdef->is_no_inherit,
+			cdef->skip_validation))
 continue;
 		}
 		else
@@ -2389,7 +2390,7 @@ AddRelationNewConstraints(Relation rel,
 static bool
 MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
 			bool allow_merge, bool is_local,
-			bool is_no_inherit)
+			bool is_no_inherit, bool is_not_valid)
 {
 	bool		found;
 	Relation	conDesc;
@@ -2452,6 +2453,14 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
 		 errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
 ccname, RelationGetRelationName(rel;
 
+			/* Cannot merge if the child's constraint is yet invalid. */
+			if (!is_local && !is_not_valid && !con->convalidated)
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+		 errmsg("cannot merge constraint \"%s\" with NOT VALID constraint on relation \"%s\"",
+ccname, RelationGetRelationName(rel)),
+		 errhint("Please validate the constraint on the child table first.")));
+
 			if (is_local)
 con->conislocal = true;
 			else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..5e72029 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10380,6 +10380,14 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
 NameStr(child_con->conname),
 RelationGetRelationName(child_rel;
 
+			/* Cannot merge if the child's constraint is yet invalid. */
+			if (parent_con->convalidated && !child_con->convalidated)
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+		 errmsg("cannot merge constraint \"%s\" with NOT VALID constraint on child table \"%s\"",
+ NameStr(parent_con->conname), RelationGetRelationName(child_rel)),
+		 errhint("Please validate the constraint on the child table first.")));
+
 			/*
 			 * OK, bump the child constraint's inheritance count.  (If we fail
 			 * later on, this change will just roll back.)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 3232cda..9f13c47 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -460,6 +460,43 @@ Check constraints:
 Inherits: nv_parent
 
 -- we leave nv_parent and children around to help test pg_dump logic
+-- prevent merge of constraint with NOT VALID constraint on a child table
+create table parent(a int);
+create table child () inherits (parent);
+alter table child add constraint chk_a check (a > 0) not valid;
+-- ok to merge a NOT VALID constraint itself
+alter table parent add constraint chk_a check (a > 0) not valid;
+NOTICE:  merging constraint "chk_a" with inherited definition
+alter table parent drop constraint chk_a;
+-- error
+alter table parent add constraint chk_a check (a > 0);
+ERROR:  cannot merge constraint "chk_a" with NOT VALID constraint on relation "child"
+HINT:  Please validate the constraint on the child table first.
+-- ok to merge 

Re: [HACKERS] Visual Studio 2015 and telemetry calls

[Craig Ringer]

On 22 July 2016 at 10:45, Michael Paquier  wrote:

> Hi all,
>
> Some people may have bumped into the following surprise for VS2015:
>
> https://yro.slashdot.org/story/16/06/10/1350245/visual-studio-2015-c-compiler-secretly-inserts-telemetry-code-into-binaries
>
> VS 2015 is adding calls to telemetry (surprise!) to track activity of
> an application...


It's already been removed.
https://www.infoq.com/news/2016/06/visual-cpp-telemetry .

Frankly I think it's a good idea, it was just stupid to do it silently and
by default especially on release builds. It didn't actually send data
anywhere unless locally enabled, though.

This is mostly a storm in a teacup, IMO, and if undesired it sounds like
it's just necessary to build with VS 2015 update 3. I don't personally see
it as particularly different to inserting dtrace events or similar.

https://www.visualstudio.com/en-us/news/releasenotes/vs2015-update3-vs

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

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/21 16:30, Etsuro Fujita wrote:

One thing I'd like to discuss is GetUserMappingById/GetUserMappingId.
Though those functions aren't used in any places, I didn't take them
out, because I thought somebody else would need them someday.  But
considering that user mapping OIDs now aren't available in RelOptInfos,
at least the former might be rather useless.  Should we keep them in core?


I thought a bit more about this.  ISTM that there isn't much value in  
the latter either, because we can use GetUserMapping and get that OID  
from the result, instead of using the latter, so I'd vote for removing  
those functions.  Attached is a patch for that.


Best regards,
Etsuro Fujita


remove-usermapping-helper-funcs.patch
Description: binary/octet-stream

-- 
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] Visual Studio 2015 and telemetry calls

[Michael Paquier]

On Fri, Jul 22, 2016 at 11:45 AM, Michael Paquier
 wrote:
> Some people may have bumped into the following surprise for VS2015:
> https://yro.slashdot.org/story/16/06/10/1350245/visual-studio-2015-c-compiler-secretly-inserts-telemetry-code-into-binaries
>
> VS 2015 is adding calls to telemetry (surprise!) to track activity of
> an application... I can't believe that everybody wants that as enabled
> and a workaround to disable it is to add notelemetry.obj to the linker
> visibly. So could we consider disabling telemetry as the default
> behavior when building Postgres? Anybody willing to enable that could
> just fork and hack the scripts in src/tools/msvc/.
>
> Thoughts?

Actually, it would not be weird to enable it in DEBUG builds but
disable it by default. After more research this can emit as well ETW
events for Windows (sort of equivalent of dtrace for Windows).
-- 
Michael


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

[HACKERS] Visual Studio 2015 and telemetry calls

[Michael Paquier]

Hi all,

Some people may have bumped into the following surprise for VS2015:
https://yro.slashdot.org/story/16/06/10/1350245/visual-studio-2015-c-compiler-secretly-inserts-telemetry-code-into-binaries

VS 2015 is adding calls to telemetry (surprise!) to track activity of
an application... I can't believe that everybody wants that as enabled
and a workaround to disable it is to add notelemetry.obj to the linker
visibly. So could we consider disabling telemetry as the default
behavior when building Postgres? Anybody willing to enable that could
just fork and hack the scripts in src/tools/msvc/.

Thoughts?
-- 
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] fixes for the Danish locale

[Tom Lane]

Jeff Janes  writes:
> On Thu, Jul 21, 2016 at 2:11 PM, Tom Lane  wrote:
>> I see that the core tests fall over in Turkish still :-(

> Turkish has never passed (at least back to 9.0).  It looks like it is
> in the stemming functions.  I don't understand why, I would think
> everything other than English would be failing those if the regression
> tests hard-code English stemming expectations but fail to arrange for
> English stemming rules.

It looks to me like the 'simple' dictionary assumes it can apply the
lowercasing rules implied by LC_CTYPE regardless of which language
it's supposedly working on.  This is probably something we should
improve sometime, but I doubt it's an easy change.

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] fixes for the Danish locale

[Jeff Janes]

On Thu, Jul 21, 2016 at 2:11 PM, Tom Lane  wrote:
> Jeff Janes  writes:
>> In Danish, the sequence 'aa' is sometimes treated as a single letter
>> which collates after 'z'.
>> Some regression tests got into 9.5, and are still in 9.6beta3, which
>> fail due to assuming they know how things will sort or compare.
>
> As of HEAD, "LANG=danish make check-world" passes for me, which it
> did not before the round of fixes I just pushed.
>
> I see that the core tests fall over in Turkish still :-(

Turkish has never passed (at least back to 9.0).  It looks like it is
in the stemming functions.  I don't understand why, I would think
everything other than English would be failing those if the regression
tests hard-code English stemming expectations but fail to arrange for
English stemming rules.

Cheers,

Jeff


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

[HACKERS] Rethinking TupleTableSlot deforming

[Andres Freund]

Hi,

I've previously mentioned (e.g. [1]) that tuple deforming is a serious
bottlneck. I've also experimented successfully [2] making
slot_deform_tuple() faster.

But nontheless, tuple deforming is still a *major* bottleneck in many
cases, if not the *the* major bottleneck.

We could partially address that by JITing the work slot_deform_tuple
does. Various people have, with good but not raving success, played with
that.

Alternatively/Additionally we can change the tuple format to make
deforming faster.


But I think the bigger issue than the above is actually that we're just
performing a lot of useless work in a number of common scenarios. We're
always deforming all columns up to the one needed. Very often that's a
lot of useless work.  I've experimented with selectively replacing
slot_getattr calls heap_getattr(), and for some queries that can yield
massive speedups. And obviously significant slowdowns in others.  That's
the case even when preceding columns are varlena and/or contain nulls.
I.e. a good chunk of the problem is storing the results of deforming,
not accessing the data.


ISTM, we need to change slots so that they contain information about
which columns are interesting. For the hot paths we'd then only ever
allow access to those columns, and we'd only ever deform them.  Combined
with the approach in [2] that allows us to deform tuples a lot more
efficiently.

What I'm basically thinking is that expression evaluation would always
make sure the slots have computed the relevant column set, and deform at
the beginning. There's some cases where we likely would still need to
fall back to a slower path (e.g. whole row refs), but that seems fine.

That then also allows us to nearly always avoid the slot_getattr() call,
and instead look at tts_values/nulls directly. The checks slot_getattr()
performs, and the call itself, are quite expensive.


What I'm thinking about is
a) a new ExecInitExpr()/ExecBuildProjectionInfo() which always compute a set of
   interesting columns.
b) replacing all accesses to tts_values/isnull with an inline
   function. In optimized builds that functions won't do anything but
   reference the relevant element, but in assert enabled builds it'd
   check whether said column is actually known to be accessed.
c) Make ExecEvalExpr(), ExecProject(), ExecQual() (and perhaps some
   other places) call the new deforming function which ensures the
   relevant columns are available.
d) Replace nearly all slot_getattr/slot_getsomeattrs calls with the
   function introduced in b).

To me it seems this work will be a good bit easier once [2] is actually
implemented instead of prototyped, because treating ExecInitExpr()
non-recursively allows to build such 'column sets' more easily /
naturally.


Comments? Alternative suggestions?


Greetings,

Andres Freund

[1] 
http://archives.postgresql.org/20160624232953.beub22r6yqux4...@alap3.anarazel.de
[2] 
http://archives.postgresql.org/message-id/20160714011850.bd5zhu35szle3n3c%40alap3.anarazel.de


-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Fri, Jul 22, 2016 at 9:02 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> One thing about my current set of patches is that I have begun adding
>> files from src/common/ to libpq's list of files. As that would be new
>> I am wondering if I should avoid doing so.
>
> Well, it could link source files from there just as easily as from the
> backend.  Not object files, though.

OK. I'll just keep things the current way then :)
-- 
Michael


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

[HACKERS] mostly null slots in hash-aggs cause performance degradation

[Andres Freund]

Hi,

To build the representative tuple for each group in hash-aggregates we
do:
static AggHashEntry
lookup_hash_entry(AggState *aggstate, TupleTableSlot *inputslot)
{
...
/* if first time through, initialize hashslot by cloning input slot */
if (hashslot->tts_tupleDescriptor == NULL)
{
MemoryContext oldContext = 
MemoryContextSwitchTo(hashslot->tts_mcxt);
/* get rid of constraints */
ExecSetSlotDescriptor(hashslot, 
CreateTupleDescCopy(inputslot->tts_tupleDescriptor));
MemoryContextSwitchTo(oldContext);
/* Make sure all unused columns are NULLs */
ExecStoreAllNullTuple(hashslot);
}

/* transfer just the needed columns into hashslot */
slot_getsomeattrs(inputslot, linitial_int(aggstate->hash_needed));
foreach(l, aggstate->hash_needed)
{
int varNumber = lfirst_int(l) - 1;

hashslot->tts_values[varNumber] = 
inputslot->tts_values[varNumber];
hashslot->tts_isnull[varNumber] = 
inputslot->tts_isnull[varNumber];
}

i.e. we have a tuple that's all null, except for the group-by columns. I
n LookupTupleHashEntry(), we form a minimal tuple of that, if the tuple
represents a new group, which is stored in the hash-table.  Then, for
comparisons, we'll deform that again in execTuplesMatch, whenever a
hash-lookup finds a pre-existing tuple (i.e. hash conflict, or
additional row in group)..


If a tuple has a couple columns, and the group by column isn't leading,
that means we'll spend a considerable amount of time forming and
deforming NULL columns. I've seen the position of the grouping column
make as much as 40% performance difference, *even if* input to the
aggregates refers a later column.


Thus it seems like we instead should have a separate targetlist for the
hash slot, only containing the grouped-by columns.


I'm not entirely sure what the best way to do that is, though.  The
simpler, and hackier, way would be to do that locally in nodeAgg.c, and
reconstruct the expected tuple again in agg_retrieve_hash_table() (where
we currently do the ExecStoreMinimalTuple()).  Alternatively we could
build a separate targetlist at createplan.c time; but the details aren't
entirely clear to me.

Comments?

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] Password identifiers, protocol aging and SCRAM protocol

[Tom Lane]

Michael Paquier  writes:
> On Fri, Jul 22, 2016 at 8:48 AM, Tom Lane  wrote:
>> I'm confused.  We need that code in both libpq and backend, no?
>> src/common is the place for stuff of that description.

> Not necessarily. src/interfaces/libpq/Makefile uses a set of files
> like md5.c which is located in the backend code and directly compiles
> libpq.so with them, so one possibility would be to do the same for
> sha.c: locate the file in src/backend/libpq/ and then fetch the file
> directly when compiling libpq's shared library.

Meh.  That seems like a hack left over from before we had src/common.

Having said that, src/interfaces/libpq/ does have some special
requirements, because it needs the code compiled with -fpic (on most
hardware), which means it can't just use the client-side libpgcommon.a
builds.  So maybe it's not worth improving this.

> One thing about my current set of patches is that I have begun adding
> files from src/common/ to libpq's list of files. As that would be new
> I am wondering if I should avoid doing so.

Well, it could link source files from there just as easily as from the
backend.  Not object files, though.

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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Fri, Jul 22, 2016 at 8:48 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Because I would like to just change my set of patches to have the SHA
>> and the encoding functions in src/backend/libpq instead of src/common,
>> and then have pgcrypto be compiled with a link to those files. That's
>> a cleaner design btw, more in line with what is done for md5..
>
> I'm confused.  We need that code in both libpq and backend, no?
> src/common is the place for stuff of that description.

Not necessarily. src/interfaces/libpq/Makefile uses a set of files
like md5.c which is located in the backend code and directly compiles
libpq.so with them, so one possibility would be to do the same for
sha.c: locate the file in src/backend/libpq/ and then fetch the file
directly when compiling libpq's shared library.

One thing about my current set of patches is that I have begun adding
files from src/common/ to libpq's list of files. As that would be new
I am wondering if I should avoid doing so. Here is what I mean:
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -43,6 +43,14 @@ OBJS += $(filter crypt.o getaddrinfo.o getpeereid.o
inet_aton.o open.o system.o
 OBJS += ip.o md5.o
 # utils/mb
 OBJS += encnames.o wchar.o
+# common/
+OBJS += encode.o scram-common.o
+
-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Tom Lane]

Michael Paquier  writes:
> On Fri, Jul 22, 2016 at 2:31 AM, Tom Lane  wrote:
>> Note that "crypto" for this purpose generally means reversible encryption;
>> I've never heard that one-way hashes are illegal anywhere.  So password
>> hashing such as md5 is fine in core, and a stronger hash would be too.
>> But pulling in pgcrypto lock, stock, and barrel is not OK.

> So it would be an issue if pgcrypto.so links directly to libpqcommon?

No, I don't see why that'd be an issue.  What we can't do is have
libpgcommon depending on pgcrypto.so, or containing anything more than
one-way-hash functionality itself.

> Because I would like to just change my set of patches to have the SHA
> and the encoding functions in src/backend/libpq instead of src/common,
> and then have pgcrypto be compiled with a link to those files. That's
> a cleaner design btw, more in line with what is done for md5..

I'm confused.  We need that code in both libpq and backend, no?
src/common is the place for stuff of that description.

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] RecoveryTargetTLI dead variable in XLogCtlData

[Michael Paquier]

On Fri, Jul 22, 2016 at 12:54 AM, Robert Haas  wrote:
> On Tue, Jul 12, 2016 at 11:29 PM, Michael Paquier
>  wrote:
>> I just bumped into $subject, a variable that is never set and never used:
>> --- a/src/backend/access/transam/xlog.c
>> +++ b/src/backend/access/transam/xlog.c
>> @@ -631,8 +631,6 @@ typedef struct XLogCtlData
>> TimeLineID  replayEndTLI;
>> /* timestamp of last COMMIT/ABORT record replayed (or being replayed) */
>> TimestampTz recoveryLastXTime;
>> -   /* current effective recovery target timeline */
>> -   TimeLineID  RecoveryTargetTLI;
>
> Committed, thanks.

Thanks.
-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Fri, Jul 22, 2016 at 2:31 AM, Tom Lane  wrote:
> Way back when, like fifteen years ago, there absolutely were US export
> control restrictions on software containing crypto.  I believe the US has
> figured out that that was silly, but I'm not sure everyplace else has.

England is these days legally running a battle against data
encryption. I have not heard how this is evolving these days.

> (And if you've been reading the news you will notice that legal
> restrictions on crypto are back in vogue, so it would not be wise to
> assume that the question is dead and buried.)  So our project policy
> since at least the turn of the century has been that any crypto facility
> has to be in a separable extension, where it would be fairly easy for
> a packager to delete it if they need to ship a crypto-free version.
> Note that "crypto" for this purpose generally means reversible encryption;
> I've never heard that one-way hashes are illegal anywhere.  So password
> hashing such as md5 is fine in core, and a stronger hash would be too.
> But pulling in pgcrypto lock, stock, and barrel is not OK.

So it would be an issue if pgcrypto.so links directly to libpqcommon?
Because that's not what I am doing now, perhaps fortunately. I moved
the sha functions to src/common. But actually but thinking more about
that, I don't need to do so because the routines of SCRAM shared
between the frontend and the backend just need to be part of libpq so
they could just be part of backend/libpq like md5.

Tom, if I get it correctly, it would not be an issue if the SHA
functions are directly part of the compiled backend like md5, right?
Because I would like to just change my set of patches to have the SHA
and the encoding functions in src/backend/libpq instead of src/common,
and then have pgcrypto be compiled with a link to those files. That's
a cleaner design btw, more in line with what is done for md5..
-- 
Michael


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

[HACKERS] Curing plpgsql's memory leaks for statement-lifespan values

[Tom Lane]

In
https://www.postgresql.org/message-id/tencent_5c738eca65bad6861aa43...@qq.com
it was pointed out that you could get an intra-function-call memory leak
from something like

LOOP
  BEGIN
EXECUTE 'bogus command';
  EXCEPTION WHEN OTHERS THEN
  END;
END LOOP;

The problem is that exec_stmt_dynexecute() loses control to the error
thrown from the bogus command, and therefore leaks its "querystr" local
variable --- which is not much of a leak, but it adds up if the loop
iterates enough times.  There are similar problems in many other places in
plpgsql.  Basically the issue is that while running a plpgsql function,
CurrentMemoryContext points to a function-lifespan context (the same as
the SPI procCxt the function is using).  We also store things such as
values of the function's variables there, so just resetting that context
is not an option.  plpgsql does have an expression-evaluation-lifespan
context for short-lived stuff, but anything that needs to live for more
or less the duration of a statement is put into the procedure-lifespan
context, where it risks becoming a leak.  (That design was fine
originally, because any error would result in abandoning function
execution and thereby cleaning up that context.  But once we invented
plpgsql exception blocks, it's not so good.)

One way we could resolve the problem is to require all plpgsql code to
use PG_TRY/PG_CATCH blocks to ensure that statement-lifespan variables
are explicitly released.  That's undesirable on pretty much every front
though: it'd be notationally ugly, prone to omissions, and not very
speedy.

Another answer is to invent a third per-function memory context intended
to hold statement-lifespan variables.  We could say that statements are
still expected to clean up their mess in normal cases, and this context is
only cleared when BEGIN/EXCEPT catches an error.  Each BEGIN/EXCEPT would
need to create and install a new such context, since otherwise it might be
clobbering statement-lifespan variables from a surrounding statement.
A more aggressive variant is to arrange things so we can automatically
reset that context after each statement, avoiding the need for explicit
pfree's of statement-lifespan variables.  I think that would mean that
not only BEGINs but also looping statements would need to install new
contexts for their controlled statements, unless they had no
pass-by-reference local values.  The traffic for creating and deleting
such contexts might be too expensive, even though it'd buy back some
retail pfree's.

I looked into whether we could actually install such a context as
CurrentMemoryContext, allowing palloc's to go there by default.  This
seems not workable: a lot of the existing palloc's really need to be
creating function-lifespan values, and there's also a problem that SPI
expects CurrentMemoryContext to be the same as its procCxt, because many
SPI functions will just set CurrentMemoryContext to equal procCxt at exit.
So in any case, use of such a context would require explicit
MemoryContextAllocs or MemoryContextSwitchTos, which is a bit annoying
notationally, but there seems little help for it.

Thoughts, better ideas?

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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Jul 21, 2016 at 04:48:37PM -0500, Jim Nasby wrote:
> On 7/21/16 11:46 AM, David Fetter wrote:
> > > > Can't you implement this as a extension?
> > Yes.  In that case, I'd want to make it a contrib extension, as it is
> > at least in theory attached to specific major versions of the backend.
> 
> Howso?

At least one of the structures it references isn't in a public API.

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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Jim Nasby]

On 7/21/16 11:46 AM, David Fetter wrote:

> Can't you implement this as a extension?

Yes.  In that case, I'd want to make it a contrib extension, as it is
at least in theory attached to specific major versions of the backend.


Howso?
--
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)   mobile: 512-569-9461


--
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] fixes for the Danish locale

[Tom Lane]

Jeff Janes  writes:
> In Danish, the sequence 'aa' is sometimes treated as a single letter
> which collates after 'z'.
> Some regression tests got into 9.5, and are still in 9.6beta3, which
> fail due to assuming they know how things will sort or compare.

As of HEAD, "LANG=danish make check-world" passes for me, which it
did not before the round of fixes I just pushed.

I see that the core tests fall over in Turkish still :-(

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] pg_restore & search_path, COPY failed for table "mytable": ERROR: function myinnerfunction(integer) does not exist

[David G. Johnston]

On Thu, Jul 21, 2016 at 1:57 PM, Jean-Pierre Pelletier <
jppellet...@e-djuster.com> wrote:

>
> I'm puzzled as to how search_path should be used,.
> Should all references be schema qualified inside functions body ?
>

​Pretty much...you can also do:

CREATE FUNCTION funcname()
SET search_path TO 'other_schemas_needed_by_this_function'
AS $$
[...]
$$​

​You don't have to specify the schema the function is going to reside
in...but there is exposure if you don't.​

Or is search_path safe except in the body of functions used in index or
> constraints ?
>

​pg_dump/pg_restore tends to be very conservative in setting search_path.
I'd say you are safe if you can successfully dump/restore and unsafe if you
cannot.

​Cross-schema dependencies can be problematic and if you are not willing to
test that your omissions are immaterial I'd say you should take the
paranoid route an schema-prefix everything - either explicitly or by taking
advantage of attribute setting options for functions.

Views, materialized and otherwise, are other areas commonly affected by lax
schema specifications.

David J.

Re: [HACKERS] fixes for the Danish locale

[Andrew Dunstan]

On 07/21/2016 02:26 PM, Greg Stark wrote:

On Thu, Jul 21, 2016 at 5:44 PM, Tom Lane  wrote:

Confirmed here.  Will deal with it, but I wonder why we have no buildfarm
members covering this ...

We're not going to have a build farm member for every locale the local
systems support.

Perhaps the build farm script should pick a random locale for each
run. Either a random locale from the set on the OS or a random
language from a list of locale that the regression tests are intended
to be safe for.




I don't see why we shouldn't have a buildfarm machine that tests a very 
large number of locales. It takes a very lightly resourced machine like 
nightjar just over two minutes per locale. The list of locales to test 
is a setting in the config file.


cheers

andrew



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

[HACKERS] pg_restore & search_path, COPY failed for table "mytable": ERROR: function myinnerfunction(integer) does not exist

[Jean-Pierre Pelletier]

Hi,

The following steps reproduce an error with pg_restore:

DROP TABLE IF EXISTS myTable;
DROP FUNCTION IF EXISTS myInnerFunction(INTEGER);
DROP FUNCTION IF EXISTS myOuterFunction(INTEGER);
DROP SCHEMA IF EXISTS myOtherSchema;

CREATE SCHEMA myOtherSchema;

SET search_path = myPrimarySchema, myOtherSchema, public;

CREATE OR REPLACE FUNCTION myOtherSchema.myInnerFunction(INTEGER) RETURNS
boolean
LANGUAGE sql IMMUTABLE STRICT
AS $$
SELECT TRUE;
$$;

CREATE OR REPLACE FUNCTION myOuterFunction(INTEGER) RETURNS boolean
LANGUAGE sql IMMUTABLE STRICT
AS $$
SELECT myInnerFunction($1);
$$;

CREATE TABLE myTable(
  mycol INTEGER,
  CONSTRAINT MyConstraint CHECK (myOuterFunction(mycol))
);

INSERT INTO myTable VALUES (1);


Do a pg_dump of myTable.

Doing a pg_restore, throws:
COPY failed for table "mytable": ERROR:  function myinnerfunction(integer)
does not exist
LINE 2:  SELECT myInnerFunction($1);
^
HINT:  No function matches the given name and argument types. You might
need to add explicit type casts.
QUERY:
SELECT myInnerFunction($1);
CONTEXT:  SQL function "myouterfunction" during inlining


Using pg_dump plain on myTable shows that search_path is set to
myPrimarySchema, pg_catalog, so is missing myOtherSchema.

I'm puzzled as to how search_path should be used,.
Should all references be schema qualified inside functions body ?
Or is search_path safe except in the body of functions used in index or
constraints ?

That's with version 9.5.3 & 9.6 beta 3.

Thanks,
Jean-Pierre Pelletier


-- 
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] fixes for the Danish locale

[Peter Geoghegan]

On Thu, Jul 21, 2016 at 11:49 AM, Jeff Janes  wrote:
> Does testing in other locales ever uncover bugs other than those in
> the tests themselves?  Is it worth trying to maintain broad coverage?

Potentially, yes. The strxfrm() inconsistency issue disproportionately
affected de_DE.utf8, for example. There were other locales that were
affected less severely, and I think the majority were not shown to be
affected at all.

That being said, it probably wouldn't have caught that particular
issue if we had broad coverage. It probably would catch a broken test,
though.

-- 
Peter Geoghegan


-- 
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] fixes for the Danish locale

[Peter Geoghegan]

On Thu, Jul 21, 2016 at 11:44 AM, Tom Lane  wrote:
> Note that there are certain locales we've deliberately chosen not to
> support in some regression tests (see e.g. plpython_unicode.sql), so
> I'm not really willing to buy into the idea that "any random locale found
> on a buildfarm animal should work" anyway.  I'm much more interested in
> supporting locales that someone cares enough about to configure a
> buildfarm animal for.

That seems like a high standard to me. Locale rules are known to
change, and are explicitly versioned by glibc, for example.

-- 
Peter Geoghegan


-- 
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] fixes for the Danish locale

[Jeff Janes]

On Thu, Jul 21, 2016 at 9:44 AM, Tom Lane  wrote:
> Jeff Janes  writes:
>> In Danish, the sequence 'aa' is sometimes treated as a single letter
>> which collates after 'z'.
>> Some regression tests got into 9.5, and are still in 9.6beta3, which
>> fail due to assuming they know how things will sort or compare.
>
> Confirmed here.  Will deal with it, but I wonder why we have no buildfarm
> members covering this ...
>

My CentOS box came with 735 locales installed, so testing all of them
on a regular basis would be quite a task.  And it doesn't help that
many of them seem to be very slow compared to C locale.

I guess the good news is that nothing I tested which was working in
9.5 is broken in 9.6, but several things which were working in 9.4 did
get broken in 9.5 and still are in 9.6.

The Danish fix will probably also fix the (very large) Norwegian family.

The Welsh (cy_GB) apparently put 'dd' after 'f', which breaks row
level security in much the same way as 'aa' does.

I think that that will cover all of the ones that were working in 9.4.

Does testing in other locales ever uncover bugs other than those in
the tests themselves?  Is it worth trying to maintain broad coverage?

Cheers,

Jeff


-- 
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] fixes for the Danish locale

[Tom Lane]

Peter Geoghegan  writes:
> On Thu, Jul 21, 2016 at 11:29 AM, Tom Lane  wrote:
>> Nah, we have a hard enough time with reproducibility of buildfarm results
>> without deliberately injecting transient failures.

> It could be pseudo-random, and so deterministic per buildfarm animal.
> That's what I did.

I'm not impressed with that proposal either --- then we don't even have
any control over what set of locales are getting tested.

Note that there are certain locales we've deliberately chosen not to
support in some regression tests (see e.g. plpython_unicode.sql), so
I'm not really willing to buy into the idea that "any random locale found
on a buildfarm animal should work" anyway.  I'm much more interested in
supporting locales that someone cares enough about to configure a
buildfarm animal for.

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] fixes for the Danish locale

[Peter Geoghegan]

On Thu, Jul 21, 2016 at 11:29 AM, Tom Lane  wrote:
>> Perhaps the build farm script should pick a random locale for each
>> run. Either a random locale from the set on the OS or a random
>> language from a list of locale that the regression tests are intended
>> to be safe for.
>
> Nah, we have a hard enough time with reproducibility of buildfarm results
> without deliberately injecting transient failures.

It could be pseudo-random, and so deterministic per buildfarm animal.
That's what I did.

-- 
Peter Geoghegan


-- 
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] fixes for the Danish locale

[Tom Lane]

Greg Stark  writes:
> On Thu, Jul 21, 2016 at 5:44 PM, Tom Lane  wrote:
>> Confirmed here.  Will deal with it, but I wonder why we have no buildfarm
>> members covering this ...

> We're not going to have a build farm member for every locale the local
> systems support.

Probably not, but Danish seems odd enough to be worth testing.  Aside
from this issue, I found one in the pltcl tests.

> Perhaps the build farm script should pick a random locale for each
> run. Either a random locale from the set on the OS or a random
> language from a list of locale that the regression tests are intended
> to be safe for.

Nah, we have a hard enough time with reproducibility of buildfarm results
without deliberately injecting transient failures.

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] fixes for the Danish locale

[Peter Geoghegan]

On Thu, Jul 21, 2016 at 11:26 AM, Greg Stark  wrote:
> Perhaps the build farm script should pick a random locale for each
> run. Either a random locale from the set on the OS or a random
> language from a list of locale that the regression tests are intended
> to be safe for.

That's more or less what I did with the amcheck regression tests.

-- 
Peter Geoghegan


-- 
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] fixes for the Danish locale

[Greg Stark]

On Thu, Jul 21, 2016 at 5:44 PM, Tom Lane  wrote:
>
> Confirmed here.  Will deal with it, but I wonder why we have no buildfarm
> members covering this ...

We're not going to have a build farm member for every locale the local
systems support.

Perhaps the build farm script should pick a random locale for each
run. Either a random locale from the set on the OS or a random
language from a list of locale that the regression tests are intended
to be safe for.

-- 
greg


-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Tom Lane]

David Steele  writes:
> On 7/21/16 12:19 PM, Robert Haas wrote:
>> On Wed, Jul 20, 2016 at 7:42 PM, Michael Paquier
>>  wrote:
 People have, in the past, expressed concerns about linking in
 pgcrypto.  Apparently, in some countries, it's a legal problem.

>>> Do you have any references? I don't see that as a problem.

>> I don't have a link to previous discussion handy, but I definitely
>> recall that it's been discussed.  I don't think that would mean that
>> libpgcrypto couldn't depend on libpgcommon, but the reverse direction
>> would make libpgcrypto essentially mandatory which I don't think is a
>> direction we want to go for both technical and legal reasons.

> I searched a few different ways and finally came up with this post from Tom:
> https://www.postgresql.org/message-id/11392.1389991...@sss.pgh.pa.us
> It's the only thing I could find, but thought it might jog something
> loose for somebody else.

Way back when, like fifteen years ago, there absolutely were US export
control restrictions on software containing crypto.  I believe the US has
figured out that that was silly, but I'm not sure everyplace else has.
(And if you've been reading the news you will notice that legal
restrictions on crypto are back in vogue, so it would not be wise to
assume that the question is dead and buried.)  So our project policy
since at least the turn of the century has been that any crypto facility
has to be in a separable extension, where it would be fairly easy for
a packager to delete it if they need to ship a crypto-free version.

Note that "crypto" for this purpose generally means reversible encryption;
I've never heard that one-way hashes are illegal anywhere.  So password
hashing such as md5 is fine in core, and a stronger hash would be too.
But pulling in pgcrypto lock, stock, and barrel is not OK.

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] One question about transformation ANY Sublinks into joins

[Dilip Kumar]

On Thu, Jul 21, 2016 at 9:53 PM, Robert Haas  wrote:

> It would need to be a Hash Semi Join rather than a Hash Join, wouldn't it?


I guess, Hash Join will do here,
because inner hash node is, on hash aggregate with group key on t2.id2,
t2.c2
and hash join condition is (t1.id1 = t2.id2) AND (t1.c1 = t2.c2).

So I think these together will make sure that we don't get duplicate tuple
for one outer record.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

On 7/21/16 12:19 PM, Robert Haas wrote:
> On Wed, Jul 20, 2016 at 7:42 PM, Michael Paquier
>  wrote:
>>> People have, in the past, expressed concerns about linking in
>>> pgcrypto.  Apparently, in some countries, it's a legal problem.
>>
>> Do you have any references? I don't see that as a problem.
> 
> I don't have a link to previous discussion handy, but I definitely
> recall that it's been discussed.  I don't think that would mean that
> libpgcrypto couldn't depend on libpgcommon, but the reverse direction
> would make libpgcrypto essentially mandatory which I don't think is a
> direction we want to go for both technical and legal reasons.

I searched a few different ways and finally came up with this post from Tom:

https://www.postgresql.org/message-id/11392.1389991...@sss.pgh.pa.us

It's the only thing I could find, but thought it might jog something
loose for somebody else.

I know that export controls have been an issue for crypto in the past
but have no idea what the current state of that is.

-- 
-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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Robert Haas]

On Thu, Jul 21, 2016 at 12:49 PM, Abhijit Menon-Sen  
wrote:
> At 2016-07-21 12:46:29 -0400, robertmh...@gmail.com wrote:
>>
>> I join with others in thinking it's a reasonable contrib module.
>
> I don't like the use of the term "empty" to describe an UPDATE or DELETE
> without a WHERE clause.

/me scratches head.

Who used that term?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Abhijit Menon-Sen]

At 2016-07-21 12:46:29 -0400, robertmh...@gmail.com wrote:
>
> I join with others in thinking it's a reasonable contrib module.

I don't like the use of the term "empty" to describe an UPDATE or DELETE
without a WHERE clause.

-- Abhijit


-- 
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: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Jul 21, 2016 at 12:51:50PM -0400, Robert Haas wrote:
> On Thu, Jul 21, 2016 at 12:49 PM, Abhijit Menon-Sen  
> wrote:
> > At 2016-07-21 12:46:29 -0400, robertmh...@gmail.com wrote:
> >>
> >> I join with others in thinking it's a reasonable contrib module.
> >
> > I don't like the use of the term "empty" to describe an UPDATE or DELETE
> > without a WHERE clause.
> 
> /me scratches head.
> 
> Who used that term?

I did out of failure to imagine another short way to describe the
situation as I was writing it up.  I'd be delighted to change it to
something else.

Best,
David.

Oh, and the bike shed should definitely be puce with blaze orange
polka dots.
-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Jul 21, 2016 at 12:46:29PM -0400, Robert Haas wrote:
> On Thu, Jul 21, 2016 at 12:39 PM, David Fetter  wrote:
> > On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote:
> >> > Please find attached a patch which makes it possible to disallow
> >> > UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
> >> > behavior, I've made the new GUCs PGC_SUSET.
> >> >
> >> > What say?
> >>
> >> DELETE FROM tbl WHERE true; ?
> >
> > I specifically left this possible so the feature when turned on allows
> > people to do updates with an always-true qualifier if that's what they
> > actually mean to do.
> >
> > In case it wasn't clear, unqualified updates and deletes are permitted
> > by default.  This patch allows people to set it so they're disallowed.
> 
> I join with others in thinking it's a reasonable contrib module.  In
> fact, I already wrote it for my 2015 PGCon tutorial.  Well, the
> "delete" part, anyway.
> 
> https://github.com/robertmhaas/introduction-to-postgresql-hacking/compare/master...robertmhaas:delete_needs_where

I'm happy to write the rest of this as a contrib module.  I hope to
get to that this evening.

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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Jul 21, 2016 at 09:21:55AM -0400, Jim Mlodgenski wrote:
> On Thu, Jul 21, 2016 at 12:57 AM, David Fetter  wrote:
> > Please find attached a patch which makes it possible to disallow
> > UPDATEs and DELETEs which lack a WHERE clause.  As this changes
> > query behavior, I've made the new GUCs PGC_SUSET.
> >
> > What say?
> >
> Can't you implement this as a extension?

Yes.  In that case, I'd want to make it a contrib extension, as it is
at least in theory attached to specific major versions of the backend.

Also, if it's not in contrib, we can basically forget about having
most people even know about it, let alone get specific separate
permission to use it in production.  That's reality, much as I would
like it not to be.

> The SQL Firewall project is already doing some similar concepts by
> catching prohibiting SQL and preventing it from executing.
> https://github.com/uptimejp/sql_firewall

That's very nice, but it illustrates my point perfectly.  The
extension is from a current respected and prolific contributor to the
community, but I had no idea that it was there, and by dint of writing
the PostgreSQL Weekly News, I keep closer tabs on external things
PostgreSQL than easily 99.9% of people who deploy it.

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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Robert Haas]

On Thu, Jul 21, 2016 at 12:39 PM, David Fetter  wrote:
> On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote:
>> > Please find attached a patch which makes it possible to disallow
>> > UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
>> > behavior, I've made the new GUCs PGC_SUSET.
>> >
>> > What say?
>>
>> DELETE FROM tbl WHERE true; ?
>
> I specifically left this possible so the feature when turned on allows
> people to do updates with an always-true qualifier if that's what they
> actually mean to do.
>
> In case it wasn't clear, unqualified updates and deletes are permitted
> by default.  This patch allows people to set it so they're disallowed.

I join with others in thinking it's a reasonable contrib module.  In
fact, I already wrote it for my 2015 PGCon tutorial.  Well, the
"delete" part, anyway.

https://github.com/robertmhaas/introduction-to-postgresql-hacking/compare/master...robertmhaas:delete_needs_where

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] fixes for the Danish locale

[Tom Lane]

Jeff Janes  writes:
> In Danish, the sequence 'aa' is sometimes treated as a single letter
> which collates after 'z'.
> Some regression tests got into 9.5, and are still in 9.6beta3, which
> fail due to assuming they know how things will sort or compare.

Confirmed here.  Will deal with it, but I wonder why we have no buildfarm
members covering this ...

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] Regression tests vs existing users in an installation

[Robert Haas]

On Mon, Jul 18, 2016 at 11:13 AM, Tom Lane  wrote:
>> I don't particularly like your suggestion of spooky action at a
>> distance between force_parallel_mode and regression_test_mode.  That
>> just seems kooky.
>
> It's certainly a judgment call as to which way is cleaner, but I don't
> understand your objection.  There are plenty of ways in which multiple
> GUCs determine a given behavior already.  Also, breaking this behavior
> into two variables would let us document the user-useful behavior (do
> this to test parallel safety of functions) in a different place from the
> developer-useful behavior (do this to make EXPLAIN lie to you, which
> surely has no possible use except for regression testing).

There are certainly cases where the behavior that you get depends on
multiple GUCs.  For example, the vacuum cost limit stuff is like that,
and so are the cost factors which control the query planner.  But in
each of those cases, each GUC has a function that is fully orthogonal
to each other GUC.  That doesn't seem to be the case here.  You're
saying that force_parallel_mode will decide whether we get behavior A,
and regression_test_mode will decide whether we get behavior B, but if
you ask for both A and B you will also get an additional behavior C
which would not have been selected by either GUC taken alone.  Because
the different settings are now non-orthogonal, there's now no way to
get A and B without C, or A and C without B.

Moreover, I don't want to end up in a situation where
regression_test_mode=on enables a score of minor behavior changes that
can't be separated out and tested individually.  It's true that
checking the names of regression roles is such a very minor thing that
a generic name like regression_test_mode might be OK, with the idea
that anything else similarly minor that comes along later can be
folded into that as well.  But I fear that it will become a crutch: my
code makes the regression test fail.  Instead of writing better tests,
add another thing that's conditional on regression_test_mode!
Eventually, we'll have a bug that the regression tests fail to catch
because regression_test_mode papers over the problem.  We're some
distance from such a situation now, of course, but I bet the
temptation to be undisciplined about hooking more behavior into that
GUC will be almost irresistible for new developers, and in some cases
experienced ones, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote:
> > Please find attached a patch which makes it possible to disallow
> > UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
> > behavior, I've made the new GUCs PGC_SUSET.
> > 
> > What say?
> 
> DELETE FROM tbl WHERE true; ?

I specifically left this possible so the feature when turned on allows
people to do updates with an always-true qualifier if that's what they
actually mean to do.

In case it wasn't clear, unqualified updates and deletes are permitted
by default.  This patch allows people to set it so they're disallowed.

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] Regression tests vs existing users in an installation

[Robert Haas]

On Mon, Jul 18, 2016 at 1:34 AM, Michael Paquier
 wrote:
> One downside of the plugin is that any users willing to do make
> installcheck would need to install it as well.

Not really.  If the only purpose of the plugin is to verify that we're
not creating regression users whose names don't start with "regress",
it should be good enough to run it for "make check" but not for "make
installcheck".  It's not there to test functionality, just to verify
that we've followed our own rules for regression tests.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] One question about transformation ANY Sublinks into joins

[Robert Haas]

On Sun, Jul 17, 2016 at 5:33 AM, Armor  wrote:
> Hi
> I run a simple SQL with latest PG：
> postgres=# explain select * from t1 where id1 in (select id2 from t2 where
> c1=c2);
>  QUERY PLAN
> 
>  Seq Scan on t1  (cost=0.00..43291.83 rows=1130 width=8)
>Filter: (SubPlan 1)
>SubPlan 1
>  ->  Seq Scan on t2  (cost=0.00..38.25 rows=11 width=4)
>Filter: (t1.c1 = c2)
> (5 rows)
>
> and the table schema are as following:
>
> postgres=# \d t1
>   Table "public.t1"
>  Column |  Type   | Modifiers
> +-+---
>  id1| integer |
>  c1 | integer |
>
> postgres=# \d t2
>   Table "public.t2"
>  Column |  Type   | Modifiers
> +-+---
>  id2| integer |
>  c2 | integer |
>
>  I find PG decide not to pull up this sublink because the whereClauses
> in this sublink refer to the Vars of parent query, for detail please check
> the function named convert_ANY_sublink_to_join in
> src/backend/optimizer/plan/subselect.c.
>  However, for such simple sublink which has no agg, no window function,
> no limit, may be we can carefully pull up the predicates in whereCluase
> which refers to the Vars of parent query, then pull up this sublink and
> produce a query plan as following:
>
> postgres=# explain select * from t1 where id1 in (select id2 from t2 where
> c1=c2);
>QUERY PLAN
> 
>  Hash Join  (cost=49.55..99.23 rows=565 width=8)
>Hash Cond: ((t1.id1 = t2.id2) AND (t1.c1 = t2.c2))
>->  Seq Scan on t1  (cost=0.00..32.60 rows=2260 width=8)
>->  Hash  (cost=46.16..46.16 rows=226 width=8)
>  ->  HashAggregate  (cost=43.90..46.16 rows=226 width=8)
>Group Key: t2.id2, t2.c2
>->  Seq Scan on t2  (cost=0.00..32.60 rows=2260 width=8)

It would need to be a Hash Semi Join rather than a Hash Join, wouldn't it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] Password identifiers, protocol aging and SCRAM protocol

[Robert Haas]

On Wed, Jul 20, 2016 at 7:42 PM, Michael Paquier
 wrote:
>> People have, in the past, expressed concerns about linking in
>> pgcrypto.  Apparently, in some countries, it's a legal problem.
>
> Do you have any references? I don't see that as a problem.

I don't have a link to previous discussion handy, but I definitely
recall that it's been discussed.  I don't think that would mean that
libpgcrypto couldn't depend on libpgcommon, but the reverse direction
would make libpgcrypto essentially mandatory which I don't think is a
direction we want to go for both technical and legal reasons.

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


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

[HACKERS] freeze map open item

[Robert Haas]

The open items list says this:

heap_update sets xmax and infomask without logging the changes or
clearing PD_ALL_VISIBLE or visibility map bits
* volunteer owner wanted
* This has been buggy essentially forever but the freeze map changes
give it new urgency.
* Andres working on committing patches

I believe this issue was fully resolved by
bfa2ab56bb8c512dc8613ee3ff0936568a1c8418.  Is that correct?
Regardless, I'd like to extend sincere and heartfelt thanks to Andres
for handling this issue, because I was sure running out of steam (and
time).

Two other issues were discussed on that thread for which no open
issues were created.  First, heap_lock_tuple failed to clear the
all-frozen status.  I believe that
eca0f1db14ac92d91d54eca8eeff2d15ccd797fa substantially addressed that
issue and therefore it should be added to the list as a resolved
issue.  But, second, as noted in further discussion on that thread and
in the commit message, there is an outstanding theoretical problem
which is non-critical but worthy of being fixed.  I recommend we open
a new open issue for that, or at least fix it.

Comments?

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


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

[HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Robert Haas]

On Wed, Jul 20, 2016 at 9:15 PM, Noah Misch  wrote:
> This PostgreSQL 9.6 open item now needs a permanent owner.  Would any other
> committer like to take ownership?  If this role interests you, please read
> this thread and the policy linked above, then send an initial status update
> bearing a date for your subsequent status update.  If the item does not have a
> permanent owner by 2016-07-24 02:00 UTC, I will resolve the item by reverting
> commit 848ef42 and followups.

I will adopt this item.  I will provide an initial patch for this
issue, or convince someone else to do so, within one week.  Therefore,
expect a further status update from me on or before July 28th.  I
expect that the patch will be based on ideas from these emails:

https://www.postgresql.org/message-id/1ab8f80a-d16e-4154-9497-98fbb1642...@anarazel.de
https://www.postgresql.org/message-id/20160720181213.f4io7gc6lyc37...@alap3.anarazel.de

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] RecoveryTargetTLI dead variable in XLogCtlData

[Robert Haas]

On Tue, Jul 12, 2016 at 11:29 PM, Michael Paquier
 wrote:
> Hi all,
>
> I just bumped into $subject, a variable that is never set and never used:
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -631,8 +631,6 @@ typedef struct XLogCtlData
> TimeLineID  replayEndTLI;
> /* timestamp of last COMMIT/ABORT record replayed (or being replayed) */
> TimestampTz recoveryLastXTime;
> -   /* current effective recovery target timeline */
> -   TimeLineID  RecoveryTargetTLI;

Committed, thanks.

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


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

[HACKERS] fixes for the Danish locale

[Jeff Janes]

In Danish, the sequence 'aa' is sometimes treated as a single letter
which collates after 'z'.

Some regression tests got into 9.5, and are still in 9.6beta3, which
fail due to assuming they know how things will sort or compare.

I thought the easiest way to deal with it was just to change the test
data to use 'ab...' rather than 'aa...' to represent an
early-collating string.

With these applied, this now passes:

LANG=danish make check

Cheers,

Jeff


danish_brin.patch
Description: Binary data


danish_row_level_security.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] Constraint merge and not valid status

[Robert Haas]

On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
 wrote:
> Consider a scenario where one adds a *valid* constraint on a inheritance
> parent which is then merged with a child table's *not valid* constraint
> during inheritance recursion.  If merged, the constraint is not checked
> for the child data even though it may have some.  Is that an oversight?

Seems like it.  I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Teodor Sigaev]

Please find attached a patch which makes it possible to disallow
UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
behavior, I've made the new GUCs PGC_SUSET.

What say?


DELETE FROM tbl WHERE true; ?

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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: Make it possible to disallow WHERE-less UPDATE and DELETE

[Joshua D. Drake]

On 07/21/2016 06:49 AM, Tom Lane wrote:

David Fetter  writes:

Please find attached a patch which makes it possible to disallow
UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
behavior, I've made the new GUCs PGC_SUSET.



What say?




-1


-1.  This is an express violation of the SQL standard, and at least the
UPDATE case has reasonable use-cases.  Moreover, if your desire is to have
training wheels for SQL, there are any number of other well-known gotchas
that are just as dangerous, for example ye olde unintentionally-correlated
subselect:
https://www.postgresql.org/message-id/20160714135233.1410.92538%40wrigleys.postgresql.org



Yes but I used to teach a weak long class on relational databases using 
PostgreSQL. The entire week I would iterate over and over and over that 
you never use an UPDATE or DELETE without a transaction. Toward the end 
of the class we would being do problem sets that included UPDATE and 
DELETE. Guess how many would trash their data because they didn't use a 
WHERE clause AND didn't use a transaction? 50%


These weren't kids, these weren't neophytes to technology. These were 
professionals, many of them programmers (PICK).



I wouldn't have any objection to an extension that enforces rules like
these, but I don't think it belongs in core.


I agree it doesn't need to be in core.

JD





regards, tom lane





--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
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: Make it possible to disallow WHERE-less UPDATE and DELETE

[Tom Lane]

David Fetter  writes:
> Please find attached a patch which makes it possible to disallow
> UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
> behavior, I've made the new GUCs PGC_SUSET.

> What say?

-1.  This is an express violation of the SQL standard, and at least the
UPDATE case has reasonable use-cases.  Moreover, if your desire is to have
training wheels for SQL, there are any number of other well-known gotchas
that are just as dangerous, for example ye olde unintentionally-correlated
subselect:
https://www.postgresql.org/message-id/20160714135233.1410.92538%40wrigleys.postgresql.org

I wouldn't have any objection to an extension that enforces rules like
these, but I don't think it belongs in core.

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] Unexpected memory usage for repeated inserts within plpgsql function

[Dilip Kumar]

On Thu, Jul 21, 2016 at 4:09 PM, happy times  wrote:

> My question: Is this problem as-designed?
>

Actually problem is in exec_stmt_dynexecute function, We make a copy of the
query string,
and before we free it, it thow an error from SPI_execute (because table
does not exist)
And this is happening in infinite loop, so we are seeing memory leak.

exec_stmt_dynexecute
{


/* copy it out of the temporary context before we clean up */

querystr = pstrdup(querystr);

}

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Michael Paquier]

On Thu, Jul 21, 2016 at 5:19 PM, Kyotaro HORIGUCHI
 wrote:
> + * minRecoveryPoint can go behind the last checkpoint's redo location when
> + * the checkpoint writes out no buffer. This does no harm to performing a
> + * recovery but such inversion seems inconsistent from the view of the
> + * callers and prevents them from knowing WAL segments needed by sane
> + * calcuation. For the reason we return the last replayed point as the
> + * backup end location. Note that it can be greater than the exact backup
> + * end location or even the minimum recovery point of pg_control at the
> + * time. This is harmless for current uses.

It does not seem an improvement to me to mention a comment regarding
minRecoveryPoint in do_pg_stop_backup, especially knowing that the
patch that we have here uses the last replayed LSN and TLI and does
not care about that anymore.

> AFAICS pg_stop_backup returns a single value of LSN. I don't know
> where this comes from, but the attached patch fixes this and adds
> a mention on the "gap". The original description mentioned
> backup_label and tablespace_map but it seems not necessary. The
> following is the new content rewritten by the patch.

No, this second patch is wrong. This part of the docs refers to
non-exclusive backups, where 3 fields are returned. So the docs are
correct.
-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Craig Ringer]

On 21 July 2016 at 15:49, Amit Kapila  wrote:

> On Thu, Jul 21, 2016 at 10:27 AM, David Fetter  wrote:
> > Folks,
> >
> > Please find attached a patch which makes it possible to disallow
> > UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
> > behavior, I've made the new GUCs PGC_SUSET.
> >
> > What say?
> >
>
> The use case for this functionality that comes to mind is to avoid
> deleting/updating all the data, if user has accidentally missed the
> WHERE clause.  Do you have other use case for this functionality?
> With this functionality, if user needs to actually delete or update
> all the rows, then he has to artificially add where clause which seems
> slightly inconvenient, but may be such cases are less.


It's a commonly requested feature. Personally I think it's kind of silly,
but I've had multiple people ask me for it or how to do it too. So whether
or not it's really effective/useful, it's in demand.

Personally I'd rather see it as part of an extension that does other
filtering, I don't find it compelling for core. But I don't really object
either.

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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Jim Mlodgenski]

On Thu, Jul 21, 2016 at 12:57 AM, David Fetter  wrote:

> Folks,
>
> Please find attached a patch which makes it possible to disallow
> UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
> behavior, I've made the new GUCs PGC_SUSET.
>
> What say?
>
>
Can't you implement this as a extension? The SQL Firewall project is
already doing some similar concepts by catching prohibiting SQL and
preventing it from executing.
https://github.com/uptimejp/sql_firewall

Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Kyotaro HORIGUCHI]

Hello,

At Mon, 11 Jul 2016 16:47:53 +0900, Michael Paquier  
wrote in 

Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Kyotaro HORIGUCHI]

Sorry for the absense. I've reached here.

At Thu, 21 Jul 2016 12:20:30 +0900, Michael Paquier  
wrote in 
> On Thu, Jul 21, 2016 at 11:56 AM, Amit Kapila  wrote:
> > On Thu, Jul 21, 2016 at 7:28 AM, Michael Paquier
> >  wrote:
> >> On Wed, Jul 20, 2016 at 8:56 PM, Michael Paquier
> >>  wrote:
> 
>  Yeah, I think that is totally different angle to fix this issue, so
>  don't you think it is better to start a separate thread to discuss
>  about it for 10.0 and mark this patch as ready for committer.
> >>>
> >>> I'd like to tackle this problem in 10.0, but that will strongly depend
> >>> on how my patches move on in CF1 and CF2.
> >>
> >> By the way, thank you for taking the time to provide input. I think
> >> we're in good shape here now.
> >>
> >
> > So, if I understand correctly, then we can mark the version posted by
> > you upthread [1] which includes a test along with Kyotaro's fix can be
> > marked as Ready for committer.  If so, then please change the status
> > of patch accordingly.
> 
> Oops. I thought you did it already. So done.

Thank you very much for the intensive discussion on this.

After some additional consideration, I attached two patches about
comment and documentation. 0001- is a patch related to the
previous patch that adds description on why we use replay end
point instead of minRecoveryPoint. And 0002- is a fix and an
additional mention on what would happen when pg_control is not
copied last during backup.


 About the first patch.

Related to the previous patch, I found the following comment just
above where it applies.

xlog.c:10412
- * We return the current minimum recovery point as the backup end
- * location. Note that it can be greater than the exact backup end
- * location if the minimum recovery point is updated after the backup of
- * pg_control. This is harmless for current uses.

I haven't gave a notice on this but this seems to be necessary to
edit so as to mention this fix and the gap like the following.

+ * minRecoveryPoint can go behind the last checkpoint's redo location when
+ * the checkpoint writes out no buffer. This does no harm to performing a
+ * recovery but such inversion seems inconsistent from the view of the
+ * callers and prevents them from knowing WAL segments needed by sane
+ * calcuation. For the reason we return the last replayed point as the
+ * backup end location. Note that it can be greater than the exact backup
+ * end location or even the minimum recovery point of pg_control at the
+ * time. This is harmless for current uses.


 About the second patch.

By the way, related to the following discussion in the upthread,

At Tue, 19 Jul 2016 14:13:36 +0900, Michael Paquier  
wrote in 
> The thing that is really annoying btw is that there will be always a
> gap between minRecoveryPoint and the actual moment where a backup
> finishes because there is no way to rely on the XLOG_BACKUP_END
> record. On top of that we can not be sure if pg_control has been
> backed up last or not. Which is why it would be cool to document that
> gap.

Even with out my previous patch, the gap between minRecoveryPoint
*when pg_control is backed up* and that (or the replay end) at
the end of backup is crucial and should be back-patched.

Addition to that, while I examined the documentation I found the
following description about pg_stop_backup in
continuous-archiving.html, which contradicts to its definition.

> In the same connection as before, issue the command:
> 
>   SELECT * FROM pg_stop_backup(false);
...
> The pg_stop_backup will return one row with three values.

AFAICS pg_stop_backup returns a single value of LSN. I don't know
where this comes from, but the attached patch fixes this and adds
a mention on the "gap". The original description mentioned
backup_label and tablespace_map but it seems not necessary. The
following is the new content rewritten by the patch.

| The pg_stop_backup will return the LSN when it is called. This
| LSN informs upto where the backup needs WAL segment files to
| complete. For a backup taken from a master, this function leaves
| a backup history file to inform the segment files needed and a
| server starts from the backup performes a recovery up to where a
| backup-end WAL record to reach a consistent state. But things are
| different for a backup taken from a standby. For the case, backup
| history file won't be created and recvoery is perfomed up to the
| Minimum-recovery-ending-location field in the pg_control file
| included in the backup instead. The location is properly stored
| in the backup if you copy the the pg_control file after all the
| other files as pg_basebackup does. Note that a hot standby
| started from a backup not taken in 

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Amit Kapila]

On Thu, Jul 21, 2016 at 10:27 AM, David Fetter  wrote:
> Folks,
>
> Please find attached a patch which makes it possible to disallow
> UPDATEs and DELETEs which lack a WHERE clause.  As this changes query
> behavior, I've made the new GUCs PGC_SUSET.
>
> What say?
>

The use case for this functionality that comes to mind is to avoid
deleting/updating all the data, if user has accidentally missed the
WHERE clause.  Do you have other use case for this functionality?
With this functionality, if user needs to actually delete or update
all the rows, then he has to artificially add where clause which seems
slightly inconvenient, but may be such cases are less.

-- 
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] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/16 6:25, Tom Lane wrote:

Pushed, after fooling around with it some more so that it would cover the
case we discussed where the join could be allowed if we restrict the plan
to be executed by the owner of a view used in the query.


I left that for future work, but I'm happy to have that in 9.6.  Thanks  
for improving and committing the patch!


One thing I'd like to discuss is GetUserMappingById/GetUserMappingId.  
Though those functions aren't used in any places, I didn't take them  
out, because I thought somebody else would need them someday.  But  
considering that user mapping OIDs now aren't available in RelOptInfos,  
at least the former might be rather useless.  Should we keep them in core?


Best regards,
Etsuro Fujita




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