[HACKERS] Re: [COMMITTERS] pgsql: Process variadic arguments consistently in json functions

[Andrew Dunstan]

On 10/26/2017 12:12 AM, Michael Paquier wrote:
> On Wed, Oct 25, 2017 at 5:24 AM, Andrew Dunstan <and...@dunslane.net> wrote:
>> Process variadic arguments consistently in json functions
>>
>> json_build_object and json_build_array and the jsonb equivalents did not
>> correctly process explicit VARIADIC arguments. They are modified to use
>> the new extract_variadic_args() utility function which abstracts away
>> the details of the call method.
>>
>> Michael Paquier, reviewed by Tom Lane and Dmitry Dolgov.
>>
>> Backpatch to 9.5 for the jsonb fixes and 9.4 for the json fixes, as
>> that's where they originated.
> - * Copyright (c) 2014-2017, PostgreSQL Global Development Group
> + * COPYRIGHT (c) 2014-2017, PostgreSQL Global Development Group
> Andrew, I have just noticed that this noise diff has crept in. You may
> want to fix that.


Argh! I see that in your v6 patch and I thought I'd caught all of it but
apparently not for master and REL_10. I wonder how that happened?

Will fix.

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] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

[Andrew Dunstan]

On 10/22/2017 04:35 PM, Michael Paquier wrote:
> On Mon, Oct 23, 2017 at 1:44 AM, Andrew Dunstan
> <andrew.duns...@2ndquadrant.com> wrote:
>
>> here's a patch that works that way, based on Michael's code.
> Patch not attached :)
> I still have a patch half-cooked, that I can send if necessary, but
> you are on it, right?


Sorry. Here it is.

cheers

andrew

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

diff --git a/src/backend/utils/fmgr/funcapi.c b/src/backend/utils/fmgr/funcapi.c
index 9c3f451..dfd5d8c 100644
--- a/src/backend/utils/fmgr/funcapi.c
+++ b/src/backend/utils/fmgr/funcapi.c
@@ -1400,3 +1400,120 @@ TypeGetTupleDesc(Oid typeoid, List *colaliases)
 
 	return tupdesc;
 }
+
+/*
+ * Extract a set of variadic argument values, types and NULL markers. This is
+ * useful for abstracting away whether or not the call has been done
+ * with an explicit VARIADIC array or a normal set of arguments, for a
+ * VARIADIC "any" function. When doing a VARIADIC call, the caller has
+ * provided one argument made of an array of keys, so deconstruct the
+ * array data before using it for the next processing.
+ * If no explicit VARIADIC call is used, just fill in the data based on
+ * all the arguments given by the caller.
+ * This function returns the number of arguments generated. In the event
+ * where the caller provided a NULL array input, return -1.
+ * nfixed is the number of non-variadic arguments to the function - these
+ * will be ignored by this function.
+ * If required by the caller, values of type "unknown" will be converted
+ * to text datums.
+ */
+int
+extract_variadic_args(FunctionCallInfo fcinfo, Datum **args,
+	  Oid **types, bool **nulls, int nfixed,
+	  bool convert_unknown_to_text)
+{
+	bool		variadic = get_fn_expr_variadic(fcinfo->flinfo);
+	Datum	   *args_res;
+	bool	   *nulls_res;
+	Oid		   *types_res;
+	int			nargs, i;
+
+	*args = NULL;
+	*types = NULL;
+	*nulls = NULL;
+
+	if (variadic)
+	{
+		ArrayType  *array_in;
+		Oid			element_type;
+		bool		typbyval;
+		char		typalign;
+		int16		typlen;
+
+		Assert(PG_NARGS() == nfixed + 1);
+
+		if (PG_ARGISNULL(nfixed))
+			return -1;
+
+		array_in = PG_GETARG_ARRAYTYPE_P(nfixed);
+		element_type = ARR_ELEMTYPE(array_in);
+
+		get_typlenbyvalalign(element_type,
+			 , , );
+		deconstruct_array(array_in, element_type, typlen, typbyval,
+		  typalign, _res, _res,
+		  );
+
+		/* All the elements of the array have the same type */
+		types_res = (Oid *) palloc0(nargs * sizeof(Oid));
+		for (i = 0; i < nargs; i++)
+			types_res[i] = element_type;
+	}
+	else
+	{
+		nargs = PG_NARGS() - nfixed;
+		Assert (nargs > 0);
+		nulls_res = (bool *) palloc0(nargs * sizeof(bool));
+		args_res = (Datum *) palloc0(nargs * sizeof(Datum));
+		types_res = (Oid *) palloc0(nargs * sizeof(Oid));
+
+		for (i = 0; i < nargs; i++)
+		{
+			nulls_res[i] = PG_ARGISNULL(i + nfixed);
+			types_res[i] = get_fn_expr_argtype(fcinfo->flinfo, i + nfixed);
+
+			if (!OidIsValid(types_res[i]))
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+		 errmsg("could not determine data type for argument %d",
+i + nfixed + 1)));
+
+			/*
+			 * Turn a constant (more or less literal) value that's of unknown
+			 * type into text if required . Unknowns come in as a cstring
+			 * pointer.
+			 */
+			if (convert_unknown_to_text && types_res[i] == UNKNOWNOID &&
+get_fn_expr_arg_stable(fcinfo->flinfo, i + nfixed))
+			{
+types_res[i] = TEXTOID;
+
+/* important for value, key cannot being NULL */
+if (PG_ARGISNULL(i + nfixed))
+	args_res[i] = (Datum) 0;
+else
+	args_res[i] =
+		CStringGetTextDatum(PG_GETARG_POINTER(i + nfixed));
+			}
+			else
+			{
+/* no conversion needed, just take the datum as given */
+args_res[i] = PG_GETARG_DATUM(i + nfixed);
+			}
+
+			if (!OidIsValid(types_res[i]) ||
+(convert_unknown_to_text && (types_res[i] == UNKNOWNOID)))
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+		 errmsg("could not determine data type for argument %d",
+i + nfixed + 1)));
+		}
+	}
+
+	/* Fill in results */
+	*args = args_res;
+	*nulls = nulls_res;
+	*types = types_res;
+
+	return nargs;
+}
diff --git a/src/include/funcapi.h b/src/include/funcapi.h
index 951af2a..0550c07 100644
--- a/src/include/funcapi.h
+++ b/src/include/funcapi.h
@@ -281,6 +281,9 @@ extern TupleTableSlot *TupleDescGetSlot(TupleDesc tupdesc);
 extern FuncCallContext *init_MultiFuncCall(PG_FUNCTION_ARGS);
 extern FuncCallContext *per_MultiFuncCall(PG_FUNCTION_ARGS);
 extern void end_MultiFuncCall(PG_FUNCTION_ARGS, FuncCallContext *funcctx);
+extern int extract_variadic_args(FunctionCallInfo fcinfo, Datum **args,
+	  Oid **types, bool **nulls, int nfixed,
+ bool convert_unknown_t

[HACKERS] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

[Andrew Dunstan]

On 10/22/2017 12:11 PM, Andrew Dunstan wrote:
>
> On 10/21/2017 07:33 PM, Michael Paquier wrote:
>> On Sun, Oct 22, 2017 at 1:43 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> I don't think collecting all the arguments is particularly special ---
>>> format() or concat() for instance could possibly use this.  You might
>>> need an option to say what to do with unknown.
>> In this case, we could just use a boolean flag to decide if TEXTOID
>> should be enforced or not.
> A generic function is going to look a little more complicated than this,
> though. The functions as coded assume that the function has a single
> variadic argument. But for it to be useful generically it really needs
> to be able to work where there are both fixed and variadic arguments (a
> la printf style functions).
>
> I guess a simple way would be to make the caller tell the function where
> the variadic arguments start, or how many to skip, something like that.
>


here's a patch that works that way, based on Michael's code.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Continuous integration on Windows?

[Andrew Dunstan]

On 10/13/2017 08:09 AM, Thomas Munro wrote:
> On Fri, Oct 13, 2017 at 1:42 PM, Andrew Dunstan
> <andrew.duns...@2ndquadrant.com> wrote:
>> Well, as you can see here the appveyor.yml file can live outside the
>> tree that's being built.
> Here's a Wiki page where I hope we can collect how-to information on
> this general topic:
>
> https://wiki.postgresql.org/wiki/Continuous_Integration
>
> I tried your appveyor.yml, and added:
>
> test_script:
>   - cd src\tools\msvc && vcregress check
>
> That much I could figure out just by reading our manual and I could
> see that it worked first time, but to make this really useful I guess
> we'd have to teach it to dump out resulting regression.diffs files and
> backtraces from core files (as the Travis CI version accessible from
> that page does).
>


I'll add some info to the wiki. Unfortunately, the tests fail on the
tablespace test because they are running as a privileged user. We need
to find a way around that, still looking into it. (See
<https://blog.2ndquadrant.com/setting-build-machine-visual-studio-2017/>
which describes how I get around that when running by hand).

cheers

andrew



-- 
Andrew Dunstanhttps://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] Still another race condition in recovery TAP tests

[Andrew Dunstan]

On 10/13/2017 01:04 AM, Noah Misch wrote:
> On Fri, Oct 06, 2017 at 05:57:24PM +0800, Craig Ringer wrote:
>> On 6 October 2017 at 14:03, Noah Misch <n...@leadboat.com> wrote:
>>> On Fri, Sep 08, 2017 at 10:32:03PM -0400, Tom Lane wrote:
>>>> (I do kinda wonder why we rolled our own RecursiveCopy; surely there's
>>>> a better implementation in CPAN?)
>>> Fewer people will test as we grow the list of modules they must first 
>>> install.
>> Meh, I don't buy that. At worst, all we have to do is provide a script
>> that fetches them, from distro repos if possible, and failing that
>> from CPAN.
>>
>> With cpanminus, that's pretty darn simple too.
> If the tree had such a script and it were reliable, then yes, it would matter
> little whether the script procured one module or five.
>
>



Not everyone has cpanminus installed either. My approach in the
buildfarm code is to lean over backwards in order to avoid non-standard
modules. For the problem at hand we use cp/xcopy, but the tree being
copied is stable so we don't run into the disappearing/changing file
problem.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Continuous integration on Windows?

[Andrew Dunstan]

On 10/12/2017 06:46 PM, Thomas Munro wrote:
> On Fri, Oct 13, 2017 at 10:57 AM, Andrew Dunstan
> <andrew.duns...@2ndquadrant.com> wrote:
>> Actually, that didn't take too long.
>>
>> No testing yet, but this runs a build successfully:
>> <https://gist.github.com/adunstan/7f18e5db33bb2d73f69ff8c9337a4e6c>
>>
>> See results at <https://ci.appveyor.com/project/AndrewDunstan/pgdevel>
> Excellent!  Thanks for looking at this, Andrew.  It's going to be
> really useful for removing surprises.
>
> It would be nice to finish up with a little library of control files
> like this for different CI vendors, possibly with some different
> options (different compilers, different word size, add valgrind, ...).
> I don't know if it would ever make sense to have standardised CI
> control files in the tree -- many projects do -- but it's very easy to
> carry a commit that adds them in development branches but drop it as
> part of the format-patch-and-post process.
>


Well, as you can see here the appveyor.yml file can live outside the
tree that's being built.


What would be good, though, would be to split build.pl into two so you
wouldn't need the auxiliary file I had to create from it.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Continuous integration on Windows?

[Andrew Dunstan]

On 10/12/2017 04:14 PM, Andrew Dunstan wrote:
>
> On 10/11/2017 11:04 PM, Thomas Munro wrote:
>> Hi hackers,
>>
>> I don't use Windows myself, but I'd rather avoid submitting patches
>> that fail to build, build with horrible warnings or blow up on that
>> fine operating system.  I think it would be neat to be able to have
>> experimental branches of PostgreSQL built and tested on Windows
>> automatically just by pushing them to a watched public git repo.  Just
>> like Travis CI and several others do for the major GNU/Linux distros,
>> it seems there is at least one Windows-based CI company that
>> generously offers free testing to open source projects:
>> https://www.appveyor.com (hat tip to Ilmari for the pointer).  I
>> wonder... has anyone here with Microsoft know-how ever tried to
>> produce an appveyor.yml file that would do a MSVC build and
>> check-world?
>>
>
> Interesting.
>
> I'm taking a look.
>
> A couple of things not in their pre-built images that we'll need are
> flex and bison. We might be able to overcome that with chocolatey, which
> is installed, haven't tested yet.
>
> getting a working appveyor.yml will take a little while, though.



Actually, that didn't take too long.

No testing yet, but this runs a build successfully:
<https://gist.github.com/adunstan/7f18e5db33bb2d73f69ff8c9337a4e6c>

See results at <https://ci.appveyor.com/project/AndrewDunstan/pgdevel>

cheers

andrew

-- 
Andrew Dunstanhttps://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] Continuous integration on Windows?

[Andrew Dunstan]

On 10/11/2017 11:04 PM, Thomas Munro wrote:
> Hi hackers,
>
> I don't use Windows myself, but I'd rather avoid submitting patches
> that fail to build, build with horrible warnings or blow up on that
> fine operating system.  I think it would be neat to be able to have
> experimental branches of PostgreSQL built and tested on Windows
> automatically just by pushing them to a watched public git repo.  Just
> like Travis CI and several others do for the major GNU/Linux distros,
> it seems there is at least one Windows-based CI company that
> generously offers free testing to open source projects:
> https://www.appveyor.com (hat tip to Ilmari for the pointer).  I
> wonder... has anyone here with Microsoft know-how ever tried to
> produce an appveyor.yml file that would do a MSVC build and
> check-world?
>


Interesting.

I'm taking a look.

A couple of things not in their pre-built images that we'll need are
flex and bison. We might be able to overcome that with chocolatey, which
is installed, haven't tested yet.

getting a working appveyor.yml will take a little while, though.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/27/2017 02:52 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> At this stage on reflection I agree it should be pulled :-(
> That seems to be the consensus, so I'll go make it happen.
>
>> I'm not happy about the idea of marking an input function as not
>> parallel safe, certainly not without a good deal of thought and
>> discussion that we don't have time for this cycle.
> I think the way forward is to do what we had as of HEAD (984c92074),
> but add the ability to transmit the blacklist table to parallel
> workers.  Since we expect the blacklist table would be empty most of
> the time, this should be close to no overhead in practice.  I concur
> that the idea of marking the relevant functions parallel-restricted is
> probably not as safe a fix as I originally thought, and it's not a
> very desirable restriction even if it did fix the problem.
>
>   


Do you have any suggestion as to how we should transmit the blacklist to
parallel workers?

cheers

andrew

-- 
Andrew Dunstanhttps://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] datetime.h defines like PM conflict with external libraries

[Andrew Dunstan]

On 10/03/2017 04:43 PM, Tom Lane wrote:
> Andres Freund <and...@anarazel.de> writes:
>> On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote:
>>> AFAICT at a quick glance these are only used in a couple of files. Maybe
>>> the defs need to be floated off to a different header with more limited
>>> inclusion?
>> Why not just rename them to PG_PM etc? If we force potential external
>> users to do some changes, we can use more unique names just as well -
>> the effort to adapt won't be meaningfully higher... IMNSHO there's not
>> much excuse for defining macros like PM globally.
> I like the new-header-file idea because it will result in minimal code
> churn and thus minimal back-patching hazards.
>
> I do *not* like "PG_PM".  For our own purposes that adds no uniqueness
> at all.  If we're to touch these symbols then I'd go for names like
> "DATETIME_PM".  Or maybe "DT_PM" ... there's a little bit of precedent
> for the DT_ prefix already.
>
>   


Yeah. If we use a prefix +1 for DT_. If we do that then I think they
should *all* be prefixed, not just the ones we know of conflicts for.

cheers

andrew

-- 
Andrew Dunstanhttps://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] datetime.h defines like PM conflict with external libraries

[Andrew Dunstan]

On 10/03/2017 03:00 PM, Andres Freund wrote:
> Hi,
>
> In my llvm jit work I'd to
>
> #undef PM
> /* include some llvm headers */
> #define PM 1
>
> because llvm has a number of functions which have an argument named PM.
> Now that works, but it's fairly ugly. Perhaps it would be a good idea to
> name these defines in a manner that's slightly less likely to conflict?
>
> Alternatively we could use #pragma push_macro() around the includes, but
> that'd be a new dependency.
>
> Better ideas?
>


AFAICT at a quick glance these are only used in a couple of files. Maybe
the defs need to be floated off to a different header with more limited
inclusion?

cheers

andrew

-- 
Andrew Dunstanhttps://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] pg_ctl kill support for KILL signal was Re: [COMMITTERS] pgsql: Add test for postmaster crash restarts.

[Andrew Dunstan]

On 10/01/2017 04:48 PM, Andres Freund wrote:
> On 2017-10-01 16:42:44 -0400, Tom Lane wrote:
>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>> On 09/30/2017 10:32 PM, Andres Freund wrote:
>>>> Heh.  I'm inclined to take it out. We could add a --use-the-force-luke
>>>> type parameter, but it doesn't seem worth it.
>>> I agree, but I think we need this discussed on -hackers. Does anyone
>>> have an objection to allowing "pg_ctl kill KILL somepid"? As Andres
>>> points out, in most places you can just call kill from the command line
>>> anyway, so disallowing it is not really a security feature. Having it
>>> would let us have portable crash restart tests.
>> +1 for portable tests, but it still seems like something we don't want
>> to encourage users to use.  What do you think of leaving it out of the
>> documentation?
> As far as I can tell we've not documented the set of acceptable signals
> anywhere but the source. I think we can just keep it that way?


As documented it's in the help text:

printf(_("\nAllowed signal names for kill:\n"));
printf("  ABRT HUP INT QUIT TERM USR1 USR2\n");


So we can leave it out of there. OTOH I'm not a huge fan of security by
obscurity. I guess this wouldn't be too bad a case.

cheers

andrew

-- 

Andrew Dunstanhttps://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] pg_ctl kill support for KILL signal was Re: [COMMITTERS] pgsql: Add test for postmaster crash restarts.

[Andrew Dunstan]

On 09/30/2017 10:32 PM, Andres Freund wrote:
> Hi,
>
> On 2017-09-30 22:28:39 -0400, Andrew Dunstan wrote:
>>>> But even after fixing that, there unfortunately is:
>>>>
>>>> static void
>>>> set_sig(char *signame)
>>>> {
>>>> …
>>>> #if 0
>>>>/* probably should NOT provide SIGKILL */
>>>>else if (strcmp(signame, "KILL") == 0)
>>>>sig = SIGKILL;
>>>> #endif
>>>>
>>>> I'm unclear on what that provision is achieving? If you can kill with
>>>> pg_ctl you can do other nasty stuff too (like just use kill instead of
>>>> pg_ctl)?
>>
>> I put it in when we rewrote pg_ctl in C many years ago, possibly out of
>> a superabundance of caution. I agree it's worth revisiting. I think the
>> idea was that there's a difference between an ordinary footgun and an
>> officially sanctioned footgun :-)
> Heh.  I'm inclined to take it out. We could add a --use-the-force-luke
> type parameter, but it doesn't seem worth it.
>
>
>


I agree, but I think we need this discussed on -hackers. Does anyone
have an objection to allowing "pg_ctl kill KILL somepid"? As Andres
points out, in most places you can just call kill from the command line
anyway, so disallowing it is not really a security feature. Having it
would let us have portable crash restart tests.

cheers

andrew


-- 

Andrew Dunstanhttps://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] alter server for foreign table

[Andrew Dunstan]

On 09/30/2017 05:14 AM, Derry Hamilton wrote:
> Just to say, yes, this would be handy. I've been using a variant of
> that hack on reporting servers, while migrating systems from
> proprietary databases.  It behaves quite gracefully when there are
> incompatible options, and it fixes up properly with DROPs as the first
> options.
>
>


I assume the proposal is to allow changing to a different server using
the same FDW. I can see all sorts of odd things happening if we allow
changing to a server of a different FDW.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Arrays of domains

[Andrew Dunstan]

On 09/29/2017 01:10 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/28/2017 05:44 PM, Tom Lane wrote:
>>> Assuming that that's going to happen for v11, I'm inclined to leave the
>>> optimization problem until the dust settles around CaseTestExpr.
>>> Does anyone feel that this can't be committed before that's addressed?
>> I'm Ok with it as long as we don't forget to revisit this.
> I decided to go ahead and build a quick optimization for this case,
> as per the attached patch that applies on top of what we previously
> discussed.  It brings us back to almost par with HEAD:
>
>   HEADPatch   + 04.patch
>
> Q15453.235 ms 5440.876 ms 5407.965 ms
> Q29340.670 ms 10191.194 ms9407.093 ms
> Q319078.457 ms18967.279 ms19050.392 ms
> Q448196.338 ms58547.531 ms48696.809 ms


Nice.

>
> Unless Andres feels this is too ugly to live, I'm inclined to commit
> the patch with this addition.  If we don't get around to revisiting
> CaseTestExpr, I think this is OK, and if we do, this will make sure
> that we consider this case in the revisit.
>
> It's probably also worth pointing out that this test case is intentionally
> chosen to be about the worst possible case for the patch.  A less-trivial
> coercion function would make the overhead proportionally less meaningful.
> There's also the point that the old code sometimes applies two layers of
> array coercion rather than one.  As an example, coercing int4[] to
> varchar(10)[] will do that.  If I replace "x::int8[]" with
> "x::varchar(10)[]" in Q2 and Q4 in this test, I get
>
>   HEADPatch (+04)
>
> Q246929.728 ms20646.003 ms
> Q4386200.286 ms   155917.572 ms
>
>       


Yeah, testing the worst case was the idea. This improvement in the
non-worst case is pretty good.

+1 for going ahead.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Arrays of domains

[Andrew Dunstan]

On 09/28/2017 05:44 PM, Tom Lane wrote:
>
> I get these query timings in a non-cassert build:
>
>   HEADPatch
>
> Q15453.235 ms 5440.876 ms
> Q29340.670 ms 10191.194 ms
> Q319078.457 ms18967.279 ms
> Q448196.338 ms58547.531 ms
>
>
[ analysis elided]
>
> Assuming that that's going to happen for v11, I'm inclined to leave the
> optimization problem until the dust settles around CaseTestExpr.
> Does anyone feel that this can't be committed before that's addressed?
>
>   


I'm Ok with it as long as we don't forget to revisit this.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Domains and arrays and composites, oh my

[Andrew Dunstan]

On 09/28/2017 01:02 PM, Tom Lane wrote:
>
>> I do think that treating a function returning a domain-over-composite
>> differently from one returning a base composite is a POLA. We'd be very
>> hard put to explain the reasons for it to an end user.
> Do you have any thoughts about how we ought to resolve that?
>
>


Not offhand. Maybe we need to revisit the decision not to modify the
executor at all. Obviously that would make the patch a good deal more
invasive ;-(  One thought I had was that we could invent a new return
type of TYPEFUNC_DOMAIN_COMPOSITE so there would be less danger of a PL
just treating it as an unconstrained base type as it might do if it saw
TYPEFUNC_COMPOSITE.

Maybe I'm wrong, but I have a strong suspicion that of we leave it like
this now we'll regret it in the future.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Arrays of domains

[Andrew Dunstan]

On 09/28/2017 01:11 PM, Tom Lane wrote:
>
>> I wonder if we need to do any benchmarking to assure ourselves that the
>> changes to ArrayCoerceExpr don't have a significant performance impact?
> That would likely be a good idea, though I'm not very sure what or
> how to benchmark.
>
>   


Some case where we only expect the current code to produce a single
ArrayCoerceExpr, I guess. say doing text[] -> int[] ?

cheers

andrew


-- 
Andrew Dunstanhttps://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] crash-recovery test vs windows

[Andrew Dunstan]

On 09/28/2017 12:29 PM, Andrew Dunstan wrote:
> The new crash-recovery check is failing pretty reliably on jacana. It's
> already excluded on MSVC machines, so I'm inclined to exclude it on Msys
> as well.
>
>

Sorry, I meant crash-restart

cheers

andrew

-- 
Andrew Dunstanhttps://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] crash-recovery test vs windows

[Andrew Dunstan]

The new crash-recovery check is failing pretty reliably on jacana. It's
already excluded on MSVC machines, so I'm inclined to exclude it on Msys
as well.


Thoughts?


cheers


andrew


-- 
Andrew Dunstanhttps://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] Domains and arrays and composites, oh my

[Andrew Dunstan]

mplex:
>
> regression=# select * from fdc();
>   fdc  
> ---
>  (1,2)
> (1 row)
>
> I think that that could be changed with only local changes in parse
> analysis, but do we want to change it?  Arguably, making fdc() act the
> same as fc() here would amount to implicitly downcasting the domain to
> its base type.  But doing so here is optional, not necessary in order to
> make the statement sane at all, and it's arguable that we shouldn't do
> that if the user didn't tell us to.  A user who does want that to happen
> can downcast explicitly:
>
> regression=# select * from cast(fdc() as complex);
>  r | i 
> ---+---
>  1 | 2
> (1 row)
>
> (For arcane syntactic reasons you can't abbreviate CAST with :: here.)
> Another point is that if you do want the domain value as a domain
> value, and not smashed to its base type, it would be hard to get at
> if the parser acts this way --- "foo.*" would end up producing the base
> rowtype, or if it didn't, we'd have some issues with the previously
> noted rule about whole-row Vars never having domain types.
>
> So there's a case to be made that this behavior is fine as-is, but
> certainly you could also argue that it's a POLA violation.
>
> Digression: one reason I'm hesitant to introduce inessential reductions
> of domains to base types is that I'm looking ahead to arrays over
> domains, which will provide a workaround for the people who complain
> that they wish 2-D arrays would work type-wise like arrays of 1-D array
> objects.  If you "create domain inta as int[]" then inta[] would act
> like an array of array objects, mostly solving the problem I think.
> But it solves the problem only because we don't consider that a domain
> is indistinguishable from its base type.  It's hard to be sure without
> having done the work yet, but I think there will be cases where being
> over-eager to treat a domain as its base type might break the behavior
> we want for that case.  So I don't want to create a precedent for that
> here.
>
> Thoughts?
>


This is a pretty nice patch, and very small indeed all things
considered. From a code point of view I have no criticism, although
maybe we need to be a bit more emphatic in the header file comments
about the unwisdom of using get_expr_result_tupdesc().

I do think that treating a function returning a domain-over-composite
differently from one returning a base composite is a POLA. We'd be very
hard put to explain the reasons for it to an end user.

I also think we shouldn't commit this until we have accompanying patches
for the core PLs, at least for plpgsql but I bet there are things that
should be fixed for the others too.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Arrays of domains

[Andrew Dunstan]

On 08/11/2017 01:17 PM, Tom Lane wrote:
> I wrote:
>> Probably a better answer is to start supporting arrays over domain
>> types.  That was left unimplemented in the original domains patch,
>> but AFAICS not for any better reason than lack of round tuits.
> Attached is a patch series that allows us to create arrays of domain
> types.  I haven't tested this in great detail, so there might be some
> additional corners of the system that need work, but it passes basic
> sanity checks.  I believe it's independent of the other patch I have
> in the commitfest for domains over composites, but I haven't tested
> for interactions there either.
>
> 01-rationalize-coercion-APIs.patch cleans up the APIs of
> coerce_to_domain() and some internal functions in parse_coerce.c so that
> we consistently pass around a CoercionContext along with CoercionForm.
> Previously, we sometimes passed an "isExplicit" boolean flag instead,
> which is strictly less information; and coerce_to_domain() didn't even get
> that, but instead had to reverse-engineer isExplicit from CoercionForm.
> That's contrary to the documentation in primnodes.h that says that
> CoercionForm only affects display and not semantics.  I don't think this
> change fixes any live bugs, but it makes things more consistent.  The
> main reason for doing it though is that now build_coercion_expression()
> receives ccontext, which it needs in order to be able to recursively
> invoke coerce_to_target_type(), as required by the next patch.
>
> 02-reimplement-ArrayCoerceExpr.patch is the core of the patch.  It changes
> ArrayCoerceExpr so that the node does not directly know any details of
> what has to be done to the individual array elements while performing the
> array coercion.  Instead, the per-element processing is represented by
> a sub-expression whose input is a source array element and whose output
> is a target array element.  This simplifies life in parse_coerce.c,
> because it can build that sub-expression by a recursive invocation of
> coerce_to_target_type(), and it allows the executor to handle the
> per-element processing as a compiled expression instead of hard-wired
> code.  This is probably about a wash or a small loss performance-wise
> for the simplest case where we just need to invoke one coercion function
> for each element.  However, there are many cases where the existing code
> ends up generating two nested ArrayCoerceExprs, one to do the type
> conversion and one to apply a typmod (length) coercion function.  In the
> new code there will be just one ArrayCoerceExpr, saving one deconstruction
> and reconstruction of the array.  If I hadn't done it like this, adding
> domains into the mix could have produced as many as three
> ArrayCoerceExprs, where the top one would have only checked domain
> constraints; that did not sound nice performance-wise, and it would have
> required a lot of code duplication as well.
>
> Finally, 03-support-arrays-of-domains.patch simply turns on the spigot
> by creating an array type in DefineDomain(), and adds some test cases.
> Given the new method of handling ArrayCoerceExpr, everything Just Works.
>
> I'll add this to the next commitfest.
>
>   
>


I've reviewed and tested the updated versions of these patches. The
patches apply but there's an apparent typo in arrayfuncs.c -
DatumGetAnyArray instead of DatumGetAnyArrayP

Some of the line breaking in argument lists for some of the code
affected by these patches is a bit bizarre. It hasn't been made worse by
these patches but it hasn't been made better either. That's especially
true of patch 1.

Patch 1 is fairly straightforward, as is patch 3. Patch 2 is fairly
complex, but it still does the one thing stated above - there's just a
lot of housekeeping that goes along with that. I couldn't see any
obvious problems with the implementation.

I wonder if we need to do any benchmarking to assure ourselves that the
changes to ArrayCoerceExpr don't have a significant performance impact?

Apart from those concerns I think this is ready to be committed.

cheers

andrew


-- 
Andrew Dunstanhttps://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] Re: ALTER enums (was Re: [COMMITTERS] pgsql: doc: first draft of Postgres 10 release notes)

[Andrew Dunstan]

On 09/27/2017 06:05 AM, Alvaro Herrera wrote:
> Bruce Momjian wrote:
>> doc:  first draft of Postgres 10 release notes
> I noticed that this item
>
> +
> +
> +Reduce locking required for adding values to enum types (Andrew Dunstan,
> +Tom Lane)
> +
> +
> +
> +Previously it was impossible to run ALTER TYPE ... ADD
> VALUE in a
> +transaction block unless the enum type was created in the same block. 
> +Now, only references to uncommitted enum values from other transactions
> +is prohibited.
> +
>
> does not really explain this change very clearly.  (The release note item
> was slightly changed later, but not in the substance.)  It says it's
> about "locking", but it is certainly not about what we normally call
> "locking" in Postgres.
>
> So, firstly I suggest it doesn't belong in the "Locking" section, but
> rather it should be under "Utility Commands" instead.  Second, I think
> it should say "Reduce transactional requirements" rather than "reduce
> locking required".
>
> I think there are further changes needed because of commits 175774d2932d
> and 01c5de88ff2, but that's not what I'm on about here, though
> discussion on that is welcome.
>
> Contrary opinions?
>


It looks like this is moot anyway, I think the consensus is to remove
the feature and try again in release 11.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/26/2017 06:04 PM, Andrew Dunstan wrote:
>
> On 09/26/2017 05:45 PM, Stephen Frost wrote:
>> I've not been following along very closely- are we sure that ripping
>> this out won't be worse than dealing with it in-place?  Will pulling it
>> out also require a post-RC1 catversion bump?
>>
>>
>
> It shouldn't do AFAIK - the function signatures weren't changed.
>


At this stage on reflection I agree it should be pulled :-(

I'm not happy about the idea of marking an input function as not
parallel safe, certainly not without a good deal of thought and
discussion that we don't have time for this cycle.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/26/2017 05:45 PM, Stephen Frost wrote:
>
> I've not been following along very closely- are we sure that ripping
> this out won't be worse than dealing with it in-place?  Will pulling it
> out also require a post-RC1 catversion bump?
>
>


It shouldn't do AFAIK - the function signatures weren't changed.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/26/2017 02:37 PM, Tom Lane wrote:
> I wrote:
>> Pushed; sorry for the delay.
> ... and the buildfarm's not too happy.  It looks like force_parallel_mode
> breaks all the regression test cases around unsafe enums; which on
> reflection is unsurprising, because parallel workers will not have access
> to the parent's blacklist hash, so they will think unsafe values are safe.
>
> Now, as long as parallel workers are read-only, perhaps this matters
> little; they would not be allowed to write unsafe values into tables
> anyway.  I'm concerned though about whether it might be possible for a
> parallel worker to return an unsafe value to the parent (in OID form)
> and then the parent writes it into a table.  If we can convince ourselves
> that's not possible, it might be okay to just turn off force_parallel_mode
> for these test cases.
>
> A safer answer would be to mark enum_in() and other callers of
> check_safe_enum_use() as parallel-restricted.  That'd require a
> post-RC1 catversion bump, which seems pretty unpleasant, but
> none of the other answers are nice either.
>
> Transmitting the blacklist hash to workers would be a good long-term
> answer, but I don't want to try to shoehorn it in for v10.
>
> Another idea is that maybe the existence of a blacklist hash should
> be enough to turn off parallel mode altogether ... but ugh.
>
> Or maybe we're back to "revert the whole feature, go back to 9.6
> behavior".
>
> Thoughts?


I think I would mark enum_in and friends as parallel-restricted. Yes I
know it would involve a cat version bump, so I'll understand if that's
not acceptable, but it seems to me the best of a bad bunch of choices.
Second choice might be turning off parallel mode if the hash exists, but
I'm unclear how that would work.

cheers

andrew

-- 

Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/25/2017 01:34 PM, David E. Wheeler wrote:
> On Sep 25, 2017, at 10:55, Andrew Dunstan <andrew.duns...@2ndquadrant.com> 
> wrote:
>
>> Let's ask a couple of users who I think are or have been actually
>> hurting on this point. Christophe and David, any opinions?
> If I understand the issue correctly, I think I’d be fine with requiring ALTER 
> TYPE ADD LABEL to be disallowed in a transaction that also CREATEs the type 
> if it’s not currently possible to reliably tell when an enum was created in a 
> transaction. Once you can do that, then by all means allow it!
>


OK, that seems to be the consensus. So let's apply the blacklist patch
and then separately remove the 'created in the same transaction' test.
We'll need to adjust the regression tests and docs accordingly.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Built-in plugin for logical decoding output

[Andrew Dunstan]

On 09/25/2017 12:48 PM, Alvaro Hernandez wrote:
>
>
> On 25/09/17 19:39, Petr Jelinek wrote:
>>
>> Well, test_decoding is not meant for production use anyway, no need for
>> middleware to support it. The pgoutput is primarily used for internal
>> replication purposes, which is why we need something with more
>> interoperability in mind in the first place. The new plugin should still
>> support publications etc though IMHO.
>>
>>>  However, having said that, and while json is a great output format
>>> for interoperability, if there's a discussion on which plugin to
>>> include
>>> next, I'd also favor one that has some more compact representation
>>> format (or that supports several formats, not only json).
>>>
>> JSON is indeed great for interoperability, if you want more compact
>> format, use either pgoutput or write something of your own or do
>> conversion to something else in your consumer. I don't think postgres
>> needs to provide 100 different formats out of the box when there is an
>> API. The JSON output does not have to be extremely chatty either btw.
>>
>
>     In my opinion, logical decoding plugins that don't come with core
> are close to worthless (don't get me wrong):
>
> - They very unlikely will be installed in managed environments (an
> area growing significantly).
> - As anything that is not in core, raises concerns by users.
> - Distribution and testing are non-trivial: many OS/archs combinations.
>
>     Given the above, I believe having a general-purpose output plugin
> in-core is critical to the use of logical decoding. As for 9.4-9.6
> there is test_decoding, and given that AWS uses it for production,
> that's kind of fine. For 10 there is at least pgoutput, which could be
> used (even though it was meant for replication). But if a new plugin
> is to be developed for 11+, one really general purpose one, I'd say
> json is not a good choice if it is the only output it would support.
> json is too verbose, and replication, if anything, needs performance
> (it is both network heavy and serialization/deserialization is quite
> expensive). Why not, if one and only one plugin would be developed for
> 11+, general purpose, do something that is, indeed, more general,
> i.e., that supports high-performance scenarios too?
>
>
>   


A general purpose lower bandwidth plugin might one supporting Protocol
Buffers. The downside is that unlike json it's not self-contained, you
need the message definitions to interpret the stream, AIUI.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/25/2017 10:42 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/25/2017 10:14 AM, Tom Lane wrote:
>>> Oh ... I did not think we were on the same page, because your patch
>>> didn't include removal of the same-transaction heuristic.  It'd be
>>> sensible to do that as a separate patch, though, to make it easier
>>> to put back if we decide we do want it.
>> I understood you to say that the blacklist patch was all we needed to do
>> for v10. That's my position, i.e. I think we can live with the heuristic
>> test for now if the blacklist patch is applied. Maybe we need to
>> document that the heuristic test can generate some false negatives when
>> testing for a type that is created in the current transaction.
> No, as I said upthread, I want the heuristic out of there.  I think the
> blacklist idea covers enough use-cases that we possibly don't need the
> same-transaction test at all.  Furthermore I'm doubtful that the heuristic
> form of the same-transaction test is adequate to satisfy the use-cases
> that the blacklist test doesn't cover.  So I think we should remove that
> test and see whether we get any complaints, and if so what the details of
> the real-world use-cases look like.
>
>   



Let's ask a couple of users who I think are or have been actually
hurting on this point. Christophe and David, any opinions?

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/25/2017 10:14 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/24/2017 07:06 PM, Tom Lane wrote:
>>> So I think we should just stop with the blacklist test for v10,
>>> and then see if we still get complaints (and exactly what they're
>>> about) so that we can judge how much more work the problem deserves.
>>> It's still ahead of where we were in previous releases, and ahead of
>>> where we'd be if we end up reverting the patch altogether.
>> That's pretty much what I was saying.
> Oh ... I did not think we were on the same page, because your patch
> didn't include removal of the same-transaction heuristic.  It'd be
> sensible to do that as a separate patch, though, to make it easier
> to put back if we decide we do want it.
>
>   


I understood you to say that the blacklist patch was all we needed to do
for v10. That's my position, i.e. I think we can live with the heuristic
test for now if the blacklist patch is applied. Maybe we need to
document that the heuristic test can generate some false negatives when
testing for a type that is created in the current transaction.

cheers

andrew

-- 

Andrew Dunstanhttps://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] visual studio 2017 build support

[Andrew Dunstan]

On 09/25/2017 12:25 AM, Haribabu Kommi wrote:
>
>
>
>
> Thanks for pointing it out, I missed to check the Build tools support
> section.
> Here I attached the updated patch with the change in documentation to 
> include the 2008 R2 SP1 operating system also.
>
>

Thanks, committed and backpatched to 9.6 It would be nice to get
buildfarm members going supporting  VS2015 and VS2017

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/24/2017 07:06 PM, Tom Lane wrote:
>
> So I think we should just stop with the blacklist test for v10,
> and then see if we still get complaints (and exactly what they're
> about) so that we can judge how much more work the problem deserves.
> It's still ahead of where we were in previous releases, and ahead of
> where we'd be if we end up reverting the patch altogether.
>
>


That's pretty much what I was saying.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/24/2017 04:37 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> OK, here's the finished patch. It has a pretty small footprint all
>> things considered, and I think it guarantees that nothing that could be
>> done in this area in 9.6 will be forbidden. That's probably enough to
>> get us to 10 without having to revert the whole thing, ISTM, and we can
>> leave any further refinement to the next release.
> I think this could do with some more work on the comments and test cases,
> but it's basically sound.
>
> What we still need to debate is whether to remove the heuristic
> type-is-from-same-transaction test, making the user-visible behavior
> simply "you must commit an ALTER TYPE ADD VALUE before you can use the
> new value".  I'm kind of inclined to do so; the fuzzy (and inadequately
> documented) behavior we'll have if we keep it doesn't seem very nice to
> me.
>
>   



I'd rather not. The failure cases are going to be vanishingly small, I
suspect, and we've already discussed how we might improve that test. If
you want to put some weasel words in the docs that might be ok.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/23/2017 06:06 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> OK, I think I'm convinced. Here's is the WIP code I put together for the
>> blacklist. I'm was looking for a place to put the init call, but since
>> it's possibly not going anywhere I stopped :-) . My initial thought
>> about substransactions was that we should ignore them for this purpose
>> (That's why I used TopTransactionContext for the table).
> For the blacklist, I agree we could just ignore subtransactions: all
> subtransaction levels are equally uncommitted for this purpose, and
> leaving entries from failed subtransactions in place seems like a
> non-issue, since they'd never be referenced again.  (Well, barring OID
> wraparound and an enum-value-OID collision while the transaction runs,
> but I think we can ignore that as having probability epsilon.)
>
> But you need to actually put the table in TopTransactionContext, not
> CurTransactionContext ;-).  Also, I don't think you need an init call
> so much as an end-of-transaction cleanup call.  Maybe call it
> AtEOXactEnum(), for consistency with other functions called in the
> same area.
>
>> w.r.t. table size - how large? I confess I haven't seen any systems with
>> more than a few hundred enum types. But even a million or two shouldn't
>> consume a huge amount of memory, should it?
> Dynahash tables are self-expanding, so I don't see a need to stress about
> that too much.  Anything in 10-100 seems reasonable for initial size.
>



OK, here's the finished patch. It has a pretty small footprint all
things considered, and I think it guarantees that nothing that could be
done in this area in 9.6 will be forbidden. That's probably enough to
get us to 10 without having to revert the whole thing, ISTM, and we can
leave any further refinement to the next release.

cheers

andrew

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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 93dca7a..1d6f774 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -32,6 +32,7 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
+#include "catalog/pg_enum.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
 #include "commands/tablecmds.h"
@@ -2126,6 +2127,7 @@ CommitTransaction(void)
 	smgrDoPendingDeletes(true);
 
 	AtCommit_Notify();
+	AtEOXact_Enum();
 	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
 	AtEOXact_on_commit_actions(true);
@@ -2405,6 +2407,7 @@ PrepareTransaction(void)
 
 	/* PREPARE acts the same as COMMIT as far as GUC is concerned */
 	AtEOXact_GUC(true, 1);
+	AtEOXact_Enum();
 	AtEOXact_SPI(true);
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true, false);
@@ -2606,6 +2609,7 @@ AbortTransaction(void)
 			 false, true);
 		smgrDoPendingDeletes(false);
 
+		AtEOXact_Enum();
 		AtEOXact_GUC(false, 1);
 		AtEOXact_SPI(false);
 		AtEOXact_on_commit_actions(false);
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index fe61d4d..3056f68 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -28,6 +28,8 @@
 #include "utils/builtins.h"
 #include "utils/catcache.h"
 #include "utils/fmgroids.h"
+#include "utils/hsearch.h"
+#include "utils/memutils.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
 
@@ -38,6 +40,8 @@ Oid			binary_upgrade_next_pg_enum_oid = InvalidOid;
 static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
 static int	sort_order_cmp(const void *p1, const void *p2);
 
+/* hash table of values added in the current transaction by AddEnumLabel */
+static HTAB *enum_blacklist = NULL;
 
 /*
  * EnumValuesCreate
@@ -460,8 +464,49 @@ restart:
 	heap_freetuple(enum_tup);
 
 	heap_close(pg_enum, RowExclusiveLock);
+
+	/* Set up the blacklist hash if required */
+	if (enum_blacklist == NULL)
+	{
+		HASHCTL hash_ctl;
+		memset(_ctl, 0, sizeof(hash_ctl));
+		hash_ctl.keysize = sizeof(Oid);
+		hash_ctl.entrysize = sizeof(Oid);
+		hash_ctl.hcxt = TopTransactionContext;
+		enum_blacklist = hash_create("Enum blacklist for current transaction",
+		   32,
+		   _ctl,
+		   HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	}
+
+	/* Add the new value to the blacklist */
+	(void) hash_search(enum_blacklist, , HASH_ENTER, NULL);
 }
 
+/* Test if the enum is on the blacklist */
+bool
+EnumBlacklisted(Oid enum_id)
+{
+	bool found;
+
+	if (enum_blacklist == NULL)
+		return false;
+
+	(void) hash_search(enum_blacklist, _id, HASH_FIND, );
+	return found;
+}
+
+/*
+ * Clean up the blacklist hash at 

Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/23/2017 03:52 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/23/2017 02:00 PM, Tom Lane wrote:
>>> So I'm back to not being sure about the path forward.  Maybe it would be
>>> all right to say "the value added by ADD VALUE can't be used in the same
>>> transaction, period".  That's still a step forward compared to the pre-v10
>>> prohibition on doing it at all.  I don't remember if there were use-cases
>>> where we really needed the exception for new-in-transaction types.
>> Well, my idea was to have the test run like this:
>>   * is the value an old one? Test txnid of tuple. If yes it's ok
>>   * is the value one created by ALTER TYPE ADD VALUE? Test
>> blacklist. If no, it's ok.
>>   * is the enum a new one? Test whitelist. If yes, it's ok.
>>   * anything else is not ok.
> My point is that if you do 1 and 3, you don't need 2.  Or if you do
> 2 and 3, you don't need 1.  But in most cases, testing the tuple
> hint bits is cheap, so you don't really want that option.
>
> In any case, what I'm worried about is the amount of bookkeeping
> overhead added by keeping a whitelist of enum-types-created-in-
> current-transaction.  That's less than trivial, especially since
> you have to account correctly for subtransactions.  And there are
> common use-cases where that table will become large.
>
>> If we just did the blacklist and stuck with our current heuristic test
>> for enum being created in the current transaction, we'd still probably
>> avoid 99% of the problems, including specifically the one that gave rise
>> to the bug report.
> True.  But I'm not sure whether the heuristic test is adding anything
> meaningful if we use a blacklist first.  The case where it could help
> is
>
>   begin;
>   create type t as enum();
>   alter type t add value 'v';
>   -- do something with 'v'
>   commit;
>
> That perhaps is worth something, but if somebody is trying to build a new
> enum type in pieces like that, doesn't it seem fairly likely that they
> might throw in an ALTER OWNER or GRANT as well?  My feeling is that the
> lesson we need to learn is that the heuristic test isn't good enough.
>
>   


OK, I think I'm convinced. Here's is the WIP code I put together for the
blacklist. I'm was looking for a place to put the init call, but since
it's possibly not going anywhere I stopped :-) . My initial thought
about substransactions was that we should ignore them for this purpose
(That's why I used TopTransactionContext for the table).

I agree the heuristic test isn't good enough, and if we can get a 100%
accurate test for the newness of the enum type then the blacklist would
be redundant.

w.r.t. table size - how large? I confess I haven't seen any systems with
more than a few hundred enum types. But even a million or two shouldn't
consume a huge amount of memory, should it?

cheers

andrew

-- 

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

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index fe61d4d..52c1271 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -28,6 +28,8 @@
 #include "utils/builtins.h"
 #include "utils/catcache.h"
 #include "utils/fmgroids.h"
+#include "utils/hsearch.h"
+#include "utils/memutils.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
 
@@ -38,6 +40,9 @@ Oid			binary_upgrade_next_pg_enum_oid = InvalidOid;
 static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
 static int	sort_order_cmp(const void *p1, const void *p2);
 
+/* hash table of values added in current transaction by AddEnumLabel */
+
+static HTAB *enum_blacklist = NULL;
 
 /*
  * EnumValuesCreate
@@ -460,8 +465,44 @@ restart:
 	heap_freetuple(enum_tup);
 
 	heap_close(pg_enum, RowExclusiveLock);
+
+	/* set up blacklist hash if required */
+	if (enum_blacklist == NULL)
+	{
+		HASHCTL hash_ctl;
+		memset(_ctl, 0, sizeof(hash_ctl));
+		hash_ctl.keysize = sizeof(Oid);
+		hash_ctl.entrysize = sizeof(Oid);
+		hash_ctl.hcxt = CurTransactionContext;
+		enum_blacklist = hash_create("Enum blacklist for current transaction",
+		   32,
+		   _ctl,
+		   HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	}
+
+	/* and add the new value to the blacklist */
+
+	(void) hash_search(enum_blacklist, , HASH_ENTER, NULL);
 }
 
+bool
+EnumBlacklisted(Oid enum_id)
+{
+	bool found;
+
+	if (enum_blacklist == NULL)
+		return false;
+
+	(void) hash_search(enum_blacklist, _id, HASH_FIND, );
+	return found;
+}
+
+void
+InitEnumBlacklist(void)
+{
+	enum_blacklist = NULL;
+}

Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/23/2017 02:00 PM, Tom Lane wrote:
> I wrote:
>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>> I see what you're saying, but my idea was slightly different. We would
>>> only add to the hashtable I had in mind at the bottom AddEnumLabel().
>>> Any other value, whether added in the current transaction or not, should
>>> be safe, AIUI.
>> Oh, I see: a list of enum values we need to blacklist, not whitelist.
>> Yes, that's a significantly better idea than mine, because in common
>> use-cases that would be empty or have a very small number of entries.
> Oh, wait a minute ... that's not where the problem is, really.  We can
> already tell reliably whether an enum value was created in the current
> transaction: the is-it-committed check in check_safe_enum_use is
> perfectly safe for that AFAICS.  The hard part of this is the part about
> "was the enum type created in the current transaction?".  We could make
> that reliable with the table I proposed of enum types created in the
> current transaction, but the possible performance drag is a concern.
>
> What I understand you to be proposing is to blacklist individual
> enum values added by ALTER ADD VALUE, but *not* values created
> aboriginally by CREATE TYPE AS ENUM.  (The latter are surely safe,
> because the type must disappear if they do.)  However, that would
> require dropping the second part of the current documentation promise:
>
>If ALTER TYPE ... ADD VALUE (the form that adds a new value to
>an enum type) is executed inside a transaction block, the new value cannot
>be used until after the transaction has been committed, except in the case
>that the enum type itself was created earlier in the same transaction.
>
> We'd have to just say "it can't be used till the transaction has been
> committed", full stop.  Otherwise we're right back into the problem of
> needing to detect whether the enum type is new in transaction.
>
>>> Maybe we should also keep a cache of whitelisted enums
>>> created in the current transaction.
>> What for?
> I now realize that what you probably meant here was to track enum *types*,
> not values, created in the current transaction.  But if we're doing that
> then we don't really need the per-enum-value-blacklist part of it.
>
> So I'm back to not being sure about the path forward.  Maybe it would be
> all right to say "the value added by ADD VALUE can't be used in the same
> transaction, period".  That's still a step forward compared to the pre-v10
> prohibition on doing it at all.  I don't remember if there were use-cases
> where we really needed the exception for new-in-transaction types.
>
>   



Well, my idea was to have the test run like this:

  * is the value an old one? Test txnid of tuple. If yes it's ok
  * is the value one created by ALTER TYPE ADD VALUE? Test
blacklist. If no, it's ok.
  * is the enum a new one? Test whitelist. If yes, it's ok.
  * anything else is not ok.

I think that would let us keep our promise.

If we just did the blacklist and stuck with our current heuristic test
for enum being created in the current transaction, we'd still probably
avoid 99% of the problems, including specifically the one that gave rise
to the bug report.

Am I missing something?


cheers


andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/23/2017 11:16 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>
>>> The immediate question is do we care to design/implement such a thing
>>> post-RC1.  I'd have to vote "no".  I think the most prudent thing to
>>> do is revert 15bc038f9 and then have another go at it during the v11
>>> cycle.
>> Sadly I agree. We've made decisions like this in the past, and I have
>> generally been supportive of them. I think this is the first time I have
>> been on the receiving end of one so late in the process :-(
> Unless you want to try writing a patch for this in the next day or two,
> I think we have to do that.  But now that I see the plan clearly, maybe
> we could get away with a post-RC1 fix.


OK, I'll give it a shot.

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/22/2017 11:19 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/22/2017 05:46 PM, Tom Lane wrote:
>>> I'm not sure if that qualifies as a stop-ship problem, but it ain't
>>> good, for sure.  We need to look at whether we should revert 15bc038f9
>>> or somehow revise its rules.
>> I wonder if we wouldn't be better
>> doing this more directly, keeping a per-transaction hash of unsafe enum
>> values (which will almost always be empty). It might even speed up the
>> check.
> Yeah, I was considering the same thing over dinner, though I'd phrase
> it oppositely: keep a list of enum type OIDs created in the current
> transaction, so that we could whitelist them.  This could maybe become
> a problem if someone created a zillion enums in one xact, though.


I see what you're saying, but my idea was slightly different. We would
only add to the hashtable I had in mind at the bottom AddEnumLabel().
Any other value, whether added in the current transaction or not, should
be safe, AIUI. Maybe we should also keep a cache of whitelisted enums
created in the current transaction.

I'm not to worried about people creating a zillion enums (or enum labels
being added for the solution I had in mind). Even a hash of a million
Oids will only consume a few megabytes, won't it?

>
> The immediate question is do we care to design/implement such a thing
> post-RC1.  I'd have to vote "no".  I think the most prudent thing to
> do is revert 15bc038f9 and then have another go at it during the v11
> cycle.
>
>   


Sadly I agree. We've made decisions like this in the past, and I have
generally been supportive of them. I think this is the first time I have
been on the receiving end of one so late in the process :-(

cheers

andrew

-- 
Andrew Dunstanhttps://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] [BUGS] BUG #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/22/2017 05:46 PM, Tom Lane wrote:
> bal...@obiserver.hu writes:
>> PostgreSQL version: 10beta4
>> testdb=# begin;
>> BEGIN
>> testdb=# create type enumtype as enum ('v1', 'v2');
>> CREATE TYPE
>> testdb=# alter type enumtype owner to testrole;
>> ALTER TYPE
>> testdb=# create table testtable (enumcolumn enumtype not null default 'v1');
>> ERROR:  unsafe use of new value "v1" of enum type enumtype
>> HINT:  New enum values must be committed before they can be used.
> Hmm, that's pretty annoying.  It's happening be

> cause check_safe_enum_use
> insists that the enum's pg_type entry not be updated-in-transaction.
> We thought that the new rules instituted by 15bc038f9 would be generally
> more convenient to use than the old ones --- but this example shows
> that, for example, pg_dump scripts involving enums often could not
> be restored inside a single transaction.
>
> I'm not sure if that qualifies as a stop-ship problem, but it ain't
> good, for sure.  We need to look at whether we should revert 15bc038f9
> or somehow revise its rules.



:-(


The only real problem comes from adding a value to an enum that has been
created in an earlier transaction and then using that enum value. What
we're doing here is essentially a heuristic test for that condition, and
we're getting some false positives. I wonder if we wouldn't be better
doing this more directly, keeping a per-transaction hash of unsafe enum
values (which will almost always be empty). It might even speed up the
check.

cheers

andrew

-- 
Andrew Dunstanhttps://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] visual studio 2017 build support

[Andrew Dunstan]

On 09/21/2017 08:16 PM, Haribabu Kommi wrote:
>
>
>
>
> I was about to commit this after a good bit of testing when I
> noticed this:
>
>     +   Building with Visual Studio 2017 is
>     supported
>     +   down to Windows 7 SP1 and Windows
>     Server 2012 R2.
>
> I was able to build on Windows Server 2008 without a problem, so I'm
> curious why we are saying it's not supported.
>
>
> Thanks for the review.
>
> From the visual studio system requirements [1], in the section of
> supported
> operating systems, it is mentioned as windows 7 SP1 and windows server
> 2012 R2 and didn't mentioned anything about 2008, because of this reason,
> I mentioned as that it supported till the above operating systems. As
> I don't
> have windows server 2008 system availability, so I didn't verify the same.
>
> The visual studio 2017 product itself is not mentioned as that it supports
> windows server 2008, can we go ahead and mention it in our documentation?
>
> [1] -
> https://www.visualstudio.com/en-us/productinfo/vs2017-system-requirements-vs
>
>


That page also says:


Microsoft Visual Studio Build Tools 2017

Also installs on Windows Server 2008 R2 SP1


So I'm inclined to adjust the documentation accordingly.

cheers

andrew


Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/21/2017 09:41 PM, Andres Freund wrote:
> On 2017-09-21 09:30:31 -0400, Tom Lane wrote:
>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>> The speed of memset is hardly going to be the dominating factor in a
>>> 'CREATE DATABASE' command, so we could certainly afford to change to
>>> plain memset calls here.
>> Another thought is that it may be time for our decennial debate about
>> whether MemSet is worth the electrons it's printed on.  I continue to
>> think that any modern compiler+libc ought to do an equivalent or better
>> optimization given just a plain memset().  If that seems to be true
>> for recent MSVC, we could consider putting an #if into c.h to define
>> MemSet as just memset for MSVC.  Maybe later that could be extended
>> to other compilers.
> +many. glibc's memset is nearly an order of magnitude faster than MemSet
> on my machine for medium+ length copies, and still 3-4 four times ~100
> bytes. And both gcc and clang optimize way the memcpy entirely when the
> length is a fixed short length.


Perhaps we need some sort of small benchmark program that we can run on
a representative set of platforms? I presume if you can make that
assertion you already have something along those lines?

cheers

andrew


-- 
Andrew Dunstanhttps://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] visual studio 2017 build support

[Andrew Dunstan]

On 08/25/2017 11:29 PM, Haribabu Kommi wrote:
>
>
> On Fri, Aug 25, 2017 at 11:27 PM, Christian Ullrich
> <ch...@chrullrich.net <mailto:ch...@chrullrich.net>> wrote:
>
> * On 2017-06-21 02:06, Haribabu Kommi wrote:
>
> Thanks for the review. Here I attached an updated patch with
> README update.
>
>
> Hello,
>
>
> Thanks for the review.
>  
>
> the most recent update to VS 2017, version 15.3, now identifies as
> "14.11" rather than "14.10" in the output of nmake /?. Simply
> adding this value to the two places that check for 14.10 in your
> patch appears to work for me.
>
>
> VS 2017 doesn't change the nmake version to 15, and it is updating
> with every minor version, so I changed the check to accept
> everything that is greater than 14.10 and eq 15, in case in future if
> VS 2017 changes the version number.
>  
>
> In a newly created project, PlatformToolset is still "v141".
> ToolsVersion is "15.0" whereas your patch uses "14.1".
>
> ISTM that the ToolsVersion has been like this in all versions of
> VS 2017; in my collection of .vcxproj files the auto-generated
> PostgreSQL projects are the only ones using "14.1".
>
>  
> Updated the Tools version to 15.0 and kept the platform toolset as
> V141, this because the toolset is version is still points
> to V141, when I create a sample project with VS 2017 and the version
> number is inline with nmake version also.
>
>
>


I was about to commit this after a good bit of testing when I noticed this:

+   Building with Visual Studio 2017 is
supported
    +   down to Windows 7 SP1 and Windows
Server 2012 R2.

I was able to build on Windows Server 2008 without a problem, so I'm
curious why we are saying it's not supported.

cheers

andrew


-- 
Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/21/2017 02:53 AM, Haribabu Kommi wrote:
>
>
> On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan
> <andrew.duns...@2ndquadrant.com
> <mailto:andrew.duns...@2ndquadrant.com>> wrote:
>
>
>
> On 09/20/2017 08:18 PM, Andrew Dunstan wrote:
> >
> > On 09/20/2017 07:54 PM, Tom Lane wrote:
> >> Andrew Dunstan <andrew.duns...@2ndquadrant.com
> <mailto:andrew.duns...@2ndquadrant.com>> writes:
> >>> It's also warning that it will copy 16 bytes to a 13 byte
> structure at
> >>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c.
> I haven't
> >>> seen any ill effects of this so far, but it seems to indicate that
> >>> something is possibly amiss on this compiler with the MemSet
> macros.
> >> That's weird.  Is it too stupid to figure out that the if() inside
> >> MemSet evaluates to constant false in these calls?  It seems
> hard to
> >> see how it would realize that the loop will write 16 bytes if
> it doesn't
> >> propagate the constant value forward.
> >>
> >> However ... on some other compilers, I've noticed that the
> compiler seems
> >> more likely to make "obvious" deductions of that sort if the
> variables in
> >> question are marked const.  Does it help if you do
> >>
> >> -            void   *_vstart = (void *) (start); \
> >> -            int             _val = (val); \
> >> -            Size    _len = (len); \
> >> +            void   * const _vstart = (void *) (start); \
> >> +            const int       _val = (val); \
> >> +            const Size      _len = (len); \
> >>
> >>
> >> I don't think there's any strong reason not to just do
> s/MemSet/memset/
> >> in these calls and nearby ones, but it would be good to
> understand just
> >> what's wrong here.  And why it's only showing up in that file;
> seems
> >> nearly certain that we have similar coding elsewhere.
> >>
> >>
> >
> > I'll test it.
> >
>
>
> Doesn't make a difference. I agree it would be good to understand
> what's
> going on.
>
>
> These warnings are not produced when the build is run in DEBUG mode.
> Because of this I didn't see these warning when I was working with VS
> 2017.
>
> This may be an issue with VS 2017 compiler.
>


Yeah, it seems like it, particularly when identical code in another
function in the same file doesn't generate a complaint.

The speed of memset is hardly going to be the dominating factor in a
'CREATE DATABASE' command, so we could certainly afford to change to
plain memset calls here.

I'll see if I can find someone to report this to :-)

cheers

andrew

-- 
Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 08:18 PM, Andrew Dunstan wrote:
>
> On 09/20/2017 07:54 PM, Tom Lane wrote:
>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>> It's also warning that it will copy 16 bytes to a 13 byte structure at
>>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't
>>> seen any ill effects of this so far, but it seems to indicate that
>>> something is possibly amiss on this compiler with the MemSet macros.
>> That's weird.  Is it too stupid to figure out that the if() inside
>> MemSet evaluates to constant false in these calls?  It seems hard to
>> see how it would realize that the loop will write 16 bytes if it doesn't
>> propagate the constant value forward.
>>
>> However ... on some other compilers, I've noticed that the compiler seems
>> more likely to make "obvious" deductions of that sort if the variables in
>> question are marked const.  Does it help if you do
>>
>> -void   *_vstart = (void *) (start); \
>> -int _val = (val); \
>> -Size_len = (len); \
>> +void   * const _vstart = (void *) (start); \
>> +const int   _val = (val); \
>> +const Size  _len = (len); \
>>
>>
>> I don't think there's any strong reason not to just do s/MemSet/memset/
>> in these calls and nearby ones, but it would be good to understand just
>> what's wrong here.  And why it's only showing up in that file; seems
>> nearly certain that we have similar coding elsewhere.
>>
>>  
>
> I'll test it.
>


Doesn't make a difference. I agree it would be good to understand what's
going on.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 07:54 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> It's also warning that it will copy 16 bytes to a 13 byte structure at
>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't
>> seen any ill effects of this so far, but it seems to indicate that
>> something is possibly amiss on this compiler with the MemSet macros.
> That's weird.  Is it too stupid to figure out that the if() inside
> MemSet evaluates to constant false in these calls?  It seems hard to
> see how it would realize that the loop will write 16 bytes if it doesn't
> propagate the constant value forward.
>
> However ... on some other compilers, I've noticed that the compiler seems
> more likely to make "obvious" deductions of that sort if the variables in
> question are marked const.  Does it help if you do
>
> - void   *_vstart = (void *) (start); \
> - int _val = (val); \
> - Size_len = (len); \
> + void   * const _vstart = (void *) (start); \
> + const int   _val = (val); \
> + const Size  _len = (len); \
>
>
> I don't think there's any strong reason not to just do s/MemSet/memset/
> in these calls and nearby ones, but it would be good to understand just
> what's wrong here.  And why it's only showing up in that file; seems
> nearly certain that we have similar coding elsewhere.
>
>   


I'll test it.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 07:32 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/20/2017 06:13 PM, Michael Paquier wrote:
>>> Those are around for some time, see here:
>>> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1jvvywd_g0wrkot+4ufqjggrv8osqbuzzxg...@mail.gmail.com
>>> But there has been no actual agreement about how to fix them..
>> Oh. Missed that.
>> My solution was going to be slightly different. I was going to enclose
>> the #ifdef'd code in a bare block and move the rte declaration inside
>> that block.
> Of the various solutions proposed in the previous thread, I think the
> most salable alternative is probably ilmari's: get rid of the variable
> and write the assert like
>
>Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY);
>
> That's a pretty minimal change and it doesn't add any cycles to the
> non-Assert case.
>
>   


I can live with that. Will do.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 06:13 PM, Michael Paquier wrote:
> On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan
> <andrew.duns...@2ndquadrant.com> wrote:
>> First, it warns about a couple of unused variables at lines 4553 and
>> 4673 of src/backend/optimizer/path/costsize.c. I think we can do a
>> little rearrangement to keep it happy there.
> Those are around for some time, see here:
> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1jvvywd_g0wrkot+4ufqjggrv8osqbuzzxg...@mail.gmail.com
> But there has been no actual agreement about how to fix them..


Oh. Missed that.

My solution was going to be slightly different. I was going to enclose
the #ifdef'd code in a bare block and move the rte declaration inside
that block.

I think the difference between these cases and other cases of the
PG_USER_FOR_ASSERTS_ONLY macro is that these variables are only even
written to by assert-enabled code, whereas in the other cases the
variable is written to by non-assert-enabed code but only read by
assert-enabled code. The Microsoft compilers don't seem to mind that so
much.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Windows warnings from VS 2017

[Andrew Dunstan]

I'm working through testing the changes to allow building with Visual
Studio 2017. I've noticed that there are some warnings that we don't see
in earlier compilers.

First, it warns about a couple of unused variables at lines 4553 and
4673 of src/backend/optimizer/path/costsize.c. I think we can do a
little rearrangement to keep it happy there.

It's also warning that it will copy 16 bytes to a 13 byte structure at
lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't
seen any ill effects of this so far, but it seems to indicate that
something is possibly amiss on this compiler with the MemSet macros.

The regression tests are currently failing on my test platform (Windows
Server 2016) because it says it can't change permissions on the
testtablespace directory. I have no idea what's going on there, still
investigating.


cheers


andrew

-- 
Andrew Dunstanhttps://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] Show backtrace when tap tests fail

[Andrew Dunstan]

On 09/19/2017 01:31 PM, Andres Freund wrote:
> Hi,
>
> I've had a couple cases where tap tests died, and I couldn't easily see
> where / why. For development of a new test I found it useful to show
> backtraces in that case - just adding a
> use Carp::Always;
> at the start of the relevant module did the trick.
>
> I'm wondering if we shouldn't always do so if the module is
> installed. I.e. have PostgresNode or something do something like
>
> # Include module showing backtraces upon failures. As it's a
> non-standard module, don't fail if not installed.
> eval { use Carp::Always; }
>
> Comments?
>


Or maybe Devel::Confess ?

In an eval you need a 'require' rather than a 'use', AFAIK.


cheers

andrew

-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Add citext_pattern_ops for citext contrib module

[Andrew Dunstan]

On 09/19/2017 02:47 PM, Andrew Dunstan wrote:
>
> On 09/19/2017 11:11 AM, Tom Lane wrote:
>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>> This seems to have upset a number or animals in the buildfarm.
>> Actually, after looking closer, my advice is just to drop the new
>> test cases involving accented letters.  It surprises me not in the
>> least that those would have nonportable behavior: probably, some
>> locales will case-fold them and some won't.
>>
>> (This does open up some questions about whether the opclass will
>> ever be of use for Alexey's original purpose :-()
> Actually, it now looks to me like the problem is we forgot to tell
> postgres that this data is in utf8. The problem appears to resolve (at
> least on my CentOS test box, where I reproduced the buildfarm error) if
> I add
>
> set client_encoding = 'utf8';
>
> to the sql file.
>
> Of course UTF8 bytes interpreted as LATIN1 will give results that are
> ... interesting.
>
> So let's try that first and see if it make the buildfarm go green. Maybe
> there's hope for Alexey's purpose after all.


*sigh* That worked in a couple of instances but crashed and burned
elsewhere. I'll just disable the multi-byte tests as Tom suggests.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Add citext_pattern_ops for citext contrib module

[Andrew Dunstan]

On 09/19/2017 11:11 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> This seems to have upset a number or animals in the buildfarm.
> Actually, after looking closer, my advice is just to drop the new
> test cases involving accented letters.  It surprises me not in the
> least that those would have nonportable behavior: probably, some
> locales will case-fold them and some won't.
>
> (This does open up some questions about whether the opclass will
> ever be of use for Alexey's original purpose :-()

Actually, it now looks to me like the problem is we forgot to tell
postgres that this data is in utf8. The problem appears to resolve (at
least on my CentOS test box, where I reproduced the buildfarm error) if
I add

set client_encoding = 'utf8';

to the sql file.

Of course UTF8 bytes interpreted as LATIN1 will give results that are
... interesting.

So let's try that first and see if it make the buildfarm go green. Maybe
there's hope for Alexey's purpose after all.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Add citext_pattern_ops for citext contrib module

[Andrew Dunstan]

On 09/19/2017 08:35 AM, Andrew Dunstan wrote:
> Add citext_pattern_ops for citext contrib module
>
> This is similar to text_pattern_ops.
>

This seems to have upset a number or animals in the buildfarm.

I could create a third output file, but I am seriously questioning the
point of all this, if we are prepared to accept any result from these
functions and operators, depending on locale. Maybe it would be better
simply to test that the result is not null and have done with it. Thoughts?

cheers

andrew

-- 
Andrew Dunstanhttps://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] Add citext_pattern_ops to citext contrib module

[Andrew Dunstan]

On 09/18/2017 12:50 PM, Tom Lane wrote:
> Alexey Chernyshov <a.chernys...@postgrespro.ru> writes:
>> Do we need expected/citext.out? It seems that only
>> expected/citext_1.out has correct output.
> Well, for me, citext.out matches the results in C locale,
> and citext_1.out matches the results in en_US.  If you don't
> satisfy both of those cases, the buildfarm will not like you.
>
>   


I'm about to pick this one up - I will handle the expected file issue.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Automatic testing of patches in commit fest

[Andrew Dunstan]

On 09/12/2017 11:30 AM, Tom Lane wrote:
> Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
>> Tom Lane wrote:
>>> Aleksander Alekseev <a.aleks...@postgrespro.ru> writes:
>>>> === Apply Failed: 29 ===
>>>> https://commitfest.postgresql.org/14/1235/ (Support arrays over domain 
>>>> types)
>>> Can you clarify what went wrong for you on that one?  I went to rebase it,
>>> but I end up with the identical patch except for a few line-numbering
>>> variations.
>> I think "git apply" refuses to apply a patch if it doesn't apply
>> exactly.  So you could use "git apply -3" (which merges) or just plain
>> old "patch" and the patch would work fine.
>> If the criteria is that strict, I think we should relax it a bit to
>> avoid punting patches for pointless reasons.  IOW I think we should at
>> least try "git apply -3".
> FWIW, I always initially apply patches with good ol' patch(1).  So for
> me, whether that way works would be the most interesting thing.  Don't
> know about other committers' workflows.



Yeah, that's what I do, too.


>
>> Also, at this point this should surely be just an experiment.
> +1 ... seems like there's enough noise here that changing patch status
> based on the results might be premature.  Still, I applaud the effort.


I think a regular report of what doesn't apply and what doesn't build
will be very useful on its own, especially if there are links to the
failure reports. When we are satisfied that we're not getting
significant numbers of false negatives over a significant period we can
talk about automating CF state changes. I agree this is nice work.


cheers

andrew

-- 
Andrew Dunstanhttps://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] pgbench tap tests & minor fixes.

[Andrew Dunstan]

On 09/11/2017 01:58 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/08/2017 09:40 AM, Tom Lane wrote:
>>> Like you, I'm a bit worried about the code for extracting an exit
>>> status from IPC::Run::run.  We'll have to keep an eye on the buildfarm
>>> for a bit.  If there's any trouble, I'd be inclined to drop it down
>>> to just success/fail rather than checking the exact exit code.
>> bowerbird seems to have been made unhappy.
> I saw that failure, but it appears to be a server-side crash:
>
> 2017-09-10 19:39:03.395 EDT [1100] LOG:  server process (PID 11464) was 
> terminated by exception 0xC005
> 2017-09-10 19:39:03.395 EDT [1100] HINT:  See C include file "ntstatus.h" for 
> a description of the hexadecimal value.
>
> Given the lack of any log outputs from process 11464, it's hard to tell
> what it was doing, but it seems not to be any of the backends running
> pgbench queries.  So maybe an autovac worker?  I dunno.  Anyway, it's
> difficult to credit that this commit caused the failure, even if it did
> happen during the new test case.  I'm inclined to write it off as another
> one of the random crashes that bowerbird seems prone to.
>
> If the failure proves repeatable, then of course we'll need to look
> more closely.
>
>       


Hmm, it had several failures and now a success. Will keep an eye on it.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pgbench tap tests & minor fixes.

[Andrew Dunstan]

On 09/08/2017 09:40 AM, Tom Lane wrote:
> Fabien COELHO <coe...@cri.ensmp.fr> writes:
>> [ pgbench-tap-12.patch ]
> Pushed, with some minor fooling with comments and after running it
> through perltidy.  (I have no opinions about Perl code formatting,
> but perltidy does ...)
>
> The only substantive change I made was to drop the test that attempted
> to connect to no-such-host.postgresql.org.  That's (a) unnecessary,
> as this is a test of pgbench not libpq; (b) likely to provoke a wide
> range of platform-specific error messages, which we'd have to account
> for given that the test is looking for specific output; and (c) likely
> to draw complaints from buildfarm owners and packagers who do not like
> test scripts that try to make random external network connections.
>
> Like you, I'm a bit worried about the code for extracting an exit
> status from IPC::Run::run.  We'll have to keep an eye on the buildfarm
> for a bit.  If there's any trouble, I'd be inclined to drop it down
> to just success/fail rather than checking the exact exit code.
>
>   



bowerbird seems to have been made unhappy.



cheers

andrew

-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Record full paths of programs sought by "configure".

[Andrew Dunstan]

On 08/07/2017 04:20 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 08/07/2017 04:07 PM, Tom Lane wrote:
>>> Sorry, I was imprecise.  What I'm suggesting is that you drop the
>>> runtime PATH-foolery and instead put this in configure's environment:
>>>
>>> PROVE=$perlpathdir/prove
>>>
>>> Otherwise you're basically lying to configure about what you're going
>>> to use, and that's always going to break eventually.
>> Hmm, you're saying this should work now? OK, I'll try it when I get a
>> minute to spare.
> I'm pretty sure it's always worked, at least in the sense that you could
> override what configure would put into Makefile.global that way.  I'm not
> quite clear on whether providing an exact path to "prove" there is enough
> to fix your problem.  If we have any places where we need to invoke the
> corresponding version of "perl", then we have more things to fix.


I'm pretty sure that's all we need. But we can find out ;-)


>
>>> Hm, yeah, the IPC::Run test would need to deal with this as well.
>>> A PROVE_PERL environment variable is one way.  Or maybe simpler,
>>> just skip the probe for IPC::Run if PROVE has been specified
>>> externally; assume the user knows what he's doing in that case.
>> WFM
> OK, I'll go make that happen.
>
>   



OK, thanks.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Record full paths of programs sought by "configure".

[Andrew Dunstan]

On 08/07/2017 04:07 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 08/07/2017 03:36 PM, Tom Lane wrote:
>>> My goodness, that's ugly.  Is it really better than injecting
>>> "PROVE=prove"?  (I'd suggest saying that to configure, not make,
>>> so that the configure log bears some resemblance to what you
>>> want done.)
>> This is what we had to do BEFORE the change in this commit. Now it's no
>> longer sufficient.
> Sorry, I was imprecise.  What I'm suggesting is that you drop the
> runtime PATH-foolery and instead put this in configure's environment:
>
> PROVE=$perlpathdir/prove
>
> Otherwise you're basically lying to configure about what you're going
> to use, and that's always going to break eventually.



Hmm, you're saying this should work now? OK, I'll try it when I get a
minute to spare.


>
>> It would certainly be better if we could tell configure a path to prove
>> and a path to the perl we need to test IPC::Run against.
> Hm, yeah, the IPC::Run test would need to deal with this as well.
> A PROVE_PERL environment variable is one way.  Or maybe simpler,
> just skip the probe for IPC::Run if PROVE has been specified
> externally; assume the user knows what he's doing in that case.


WFM

> Are there any other gotchas in the build sequence?
>


Not that I can think of.

> Do we have/need any explicit references to the test version of "perl",
> or is "prove" a sufficient API?
>
>   


Probably sufficient.

cheers

andrew


-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Record full paths of programs sought by "configure".

[Andrew Dunstan]

On 08/07/2017 03:36 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 08/07/2017 03:21 PM, Tom Lane wrote:
>>> I'm confused.  AFAIK, that commit did not change which "prove" would
>>> be used --- at least not unless you change PATH between configure and
>>> make.  It only changed how specifically that program would be named in
>>> Makefile.global.  Please clarify how that broke anything.
>> That's exactly what we do. See
>> <https://github.com/PGBuildFarm/client-code/blob/master/run_build.pl> at
>> line 1649.
> My goodness, that's ugly.  Is it really better than injecting
> "PROVE=prove"?  (I'd suggest saying that to configure, not make,
> so that the configure log bears some resemblance to what you
> want done.)
>
>   



This is what we had to do BEFORE the change in this commit. Now it's no
longer sufficient.

It would certainly be better if we could tell configure a path to prove
and a path to the perl we need to test IPC::Run against.

e.g. PROVE=/usr/bin/prove PROVE_PERL=/usr/bin/perl configure ...

The problem in all this is that we're assuming incorrectly that the perl
we use to build against is the same as the perl we need to run the build
with. On Msys that's emphatically not true.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Record full paths of programs sought by "configure".

[Andrew Dunstan]

On 08/07/2017 03:21 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 07/31/2017 01:02 PM, Tom Lane wrote:
>>> Record full paths of programs sought by "configure".
>> The problem with this commit, as jacana is demonstrating, is that on
>> Msys it finds the wrong prove. configure needs to run against the perl
>> we build against, i.e. a native Windows perl, but prove needs to run
>> with the perl from the MSys DTK that understands MSys virtualized
>> paths. I have a hack that will allow the buildfarm to overcome the
>> difficulty, (essentially it passes 'PROVE=prove' to make) but that's
>> fairly ugly and certainly non-intuitive for someone running an MSys
>> build and TAP tests without the buildfarm client.
> I'm confused.  AFAIK, that commit did not change which "prove" would
> be used --- at least not unless you change PATH between configure and
> make.  It only changed how specifically that program would be named in
> Makefile.global.  Please clarify how that broke anything.
>
>   




That's exactly what we do. See
<https://github.com/PGBuildFarm/client-code/blob/master/run_build.pl> at
line 1649.

cheers

andrew


-- 
Andrew Dunstanhttps://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] Re: [COMMITTERS] pgsql: Record full paths of programs sought by "configure".

[Andrew Dunstan]

On 07/31/2017 01:02 PM, Tom Lane wrote:
> Record full paths of programs sought by "configure".
>
> Previously we had a mix of uses of AC_CHECK_PROG[S] and AC_PATH_PROG[S].
> The only difference between those macros is that the latter emits the
> full path to the program it finds, eg "/usr/bin/prove", whereas the
> former emits just "prove".  Let's standardize on always emitting the
> full path; this is better for documentation of the build, and it might
> prevent some types of failures if later build steps are done with
> a different PATH setting.
>
> I did not touch the AC_CHECK_PROG[S] calls in ax_pthread.m4 and
> ax_prog_perl_modules.m4.  There seems no need to make those diverge from
> upstream, since we do not record the programs sought by the former, while
> the latter's call to AC_CHECK_PROG(PERL,...) will never be reached.
>
> Discussion: https://postgr.es/m/25937.1501433...@sss.pgh.pa.us


The problem with this commit, as jacana is demonstrating, is that on Msys it 
finds the wrong prove. configure needs to run against the perl we build 
against, i.e. a native Windows perl, but prove needs to run with the perl from 
the MSys DTK that understands MSys virtualized paths. I have a hack that will 
allow the buildfarm to overcome the difficulty, (essentially it passes 
'PROVE=prove' to make) but that's fairly ugly and certainly non-intuitive for 
someone running an MSys build and TAP tests without the buildfarm client.

cheers

andrew 


-- 
Andrew Dunstanhttps://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] PL_stashcache, or, what's our minimum Perl version?

[Andrew Dunstan]

On 07/31/2017 06:54 PM, Tom Lane wrote:
> but could we do something like
> my $pflags = "PROVE_FLAGS='" . ($ENV{PROVE_FLAGS} || "--timer") . "'";
>
> to allow overriding this choice from the buildfarm config?
>
>


I have committed this in a slightly different form.

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

Re: [HACKERS] PL_stashcache, or, what's our minimum Perl version?

[Andrew Dunstan]

On 07/28/2017 08:22 AM, Andrew Dunstan wrote:
>
> On 07/27/2017 11:58 PM, Tom Lane wrote:
>> I kinda suspect we're not actively testing non-MULTIPLICITY builds
>> either.  The 5.8.7 test I just ran was with a non-MULTIPLICITY build,
>> so the case doesn't seem actively broken, but I doubt there is any
>> buildfarm coverage.  I wonder if it'd be worth getting the buildfarm
>> to log the output of "perl -V" so we could get a clearer picture
>> of what's being tested.
>>
> It's quite possible, but in general it will need a new buildfarm client 
> release. If you choose a few possibly critical animals we can ask the owners 
> to apply a fairly simple patch.


Looks like this, bit it's rather tedious. I think I might back it out. I
guess I could also write it to a log file, if we really think we need it.

<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake=2017-07-28%2018%3A37%3A19>

cheers

andrew

-- 
Andrew Dunstanhttps://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] PL_stashcache, or, what's our minimum Perl version?

[Andrew Dunstan]

On 07/27/2017 11:58 PM, Tom Lane wrote:
>
> I kinda suspect we're not actively testing non-MULTIPLICITY builds
> either.  The 5.8.7 test I just ran was with a non-MULTIPLICITY build,
> so the case doesn't seem actively broken, but I doubt there is any
> buildfarm coverage.  I wonder if it'd be worth getting the buildfarm
> to log the output of "perl -V" so we could get a clearer picture
> of what's being tested.
>
It's quite possible, but in general it will need a new buildfarm client 
release. If you choose a few possibly critical animals we can ask the owners to 
apply a fairly simple patch.

cheers

andrew



-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/27/2017 04:33 PM, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> How about we fix it like this?
> That seems pretty invasive; I'm not excited about breaking a lot of
> unrelated code (particularly third-party extensions) for plperl's benefit.
> Even if we wanted to do that in HEAD, it seems like a nonstarter for
> released branches.
>
> An even bigger issue is that if Perl feels free to redefine sigsetjmp,
> what other libc calls might they decide to horn in on?
>
> So I was trying to figure a way to not include XSUB.h except in a very
> limited part of plperl, like ideally just the .xs files.  It's looking
> like that would take nontrivial refactoring though :-(.  Another problem
> is that this critical bit of the library API is in XSUB.h:
>
> #if defined(PERL_IMPLICIT_CONTEXT) && !defined(PERL_NO_GET_CONTEXT) && 
> !defined(PERL_CORE)
> #  undef aTHX
> #  undef aTHX_
> #  define aTHXPERL_GET_THX
> #  define aTHX_   aTHX,
> #endif
>
> As best I can tell, that's absolute brain death on the part of the Perl
> crew; it means you can't write working calling code at all without
> including XSUB.h, or at least copying-and-pasting this bit out of it.
>
>   

That's the sort of thing that prompted me to ask what was the minimal
set of defines required to fix the original problem (assuming such a
thing exists)

We haven't used PERL_IMPLICIT_CONTEXT to date, and without ill effect.
For example. it's in the ExtUtils::Embed::ccopts for the perl that
jacana and bowerbird happily build and test against.

cheers

andrew


-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/27/2017 12:34 PM, Ashutosh Sharma wrote:
> On Thu, Jul 27, 2017 at 7:51 PM, Tom Lane <t...@sss.pgh.pa.us
> <mailto:t...@sss.pgh.pa.us>> wrote:
> > Ashutosh Sharma <ashu.coe...@gmail.com
> <mailto:ashu.coe...@gmail.com>> writes:
> >> Anyways, attached is the patch that corrects this issue. The patch now
> >> imports all the switches used by perl into plperl module but, after
> >> doing so, i am seeing some compilation errors on Windows. Following is
> >> the error observed,
> >
> >> SPI.obj : error LNK2019: unresolved external symbol PerlProc_setjmp
> >> referenced in function do_plperl_return_next
> >
> > That's certainly a mess, but how come that wasn't happening before?
>
> Earlier we were using Perl-5.20 version which i think didn't have
> handshaking mechanism. From perl-5.22 onwards, the functions like
> Perl_xs_handshake() or HS_KEY were introduced for handshaking purpose
> and to ensure that the handshaking between plperl and perl doesn't
> fail, we are now trying to import the switches used by  perl into
> plperl. As a result of this, macros like PERL_IMPLICIT_SYS is getting
> defined in plperl which eventually opens the following definitions
> from XSUB.h resulting in the compilation error.
>
>499 #if defined(PERL_IMPLICIT_SYS) && !defined(PERL_CORE)
> 518 #undef ioctl
> 519 #undef getlogin
> 520*#undef setjmp*
> ...
> ...
>
> 651 #define times   PerlProc_times
> 652 #define waitPerlProc_wait
> 653 *#define setjmp  PerlProc_setjmp
>
> *



What is the minimal set of extra defines required to sort out the
handshake fingerprint issue?

cheers

andrew

-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/26/2017 11:12 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>
>>> I still find this pretty ugly :-(.
>> I'm open to alternative suggestions. But I don't want to spend a heck of
>> a lot of time on this.
> Well, let's at least do the temp files a little more safely.
> Maybe like this?
>
>   


OK  tested with that and it works.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/26/2017 09:33 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>
>> The latter fact makes me
>> theorize that the problem arises from the fairly ancient perl that Msys
>> provides, and which we need to use to run prove so the TAP tests
>> understand the environment's virtualized file paths.
> It might be interesting to look into its copy of IPC::Run and see if
> the wait technology is different from later versions.


It's using 0.94 just like my Linux box.

>
>> The attached patch should get around the problem without upsetting the
>> good work you've been doing in reducing TAP test times generally.
> I still find this pretty ugly :-(.
>
>   

I'm open to alternative suggestions. But I don't want to spend a heck of
a lot of time on this.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/25/2017 02:45 PM, Andrew Dunstan wrote:
>> Anyway, if we believe that it broke with f13ea95f9, hen it's plausible
>> that CMD.EXE has been sharing pg_ctl's stdout/stderr all along, and we
>> just had not noticed before.  Could you check what happens if we
>> change the bInheritHandles parameter as I suggested upthread?
>>
>>  
>
> I'll try when I get all the animals caught up.
>


This made no difference. And I'm not really surprised, since the test is
not hanging when run from an MSVC build. The latter fact makes me
theorize that the problem arises from the fairly ancient perl that Msys
provides, and which we need to use to run prove so the TAP tests
understand the environment's virtualized file paths.


The attached patch should get around the problem without upsetting the
good work you've been doing in reducing TAP test times generally.

cheers

andrew

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

diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index 9c3551f..3acc80e 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -32,9 +32,17 @@ else
 	print $conf "listen_addresses = '127.0.0.1'\n";
 }
 close $conf;
-command_like([ 'pg_ctl', 'start', '-D', "$tempdir/data",
-	'-l', "$TestLib::log_path/001_start_stop_server.log" ],
-	qr/done.*server started/s, 'pg_ctl start');
+my $ctlcmd = [ 'pg_ctl', 'start', '-D', "$tempdir/data",
+			   '-l', "$TestLib::log_path/001_start_stop_server.log" ];
+if ($Config{osname} ne 'msys')
+{
+	command_like($ctlcmd, qr/done.*server started/s, 'pg_ctl start');
+}
+else
+{
+	# use the version of command_like that doesn't hang on Msys here
+	command_like_safe($ctlcmd, qr/done.*server started/s, 'pg_ctl start');
+}
 
 # sleep here is because Windows builds can't check postmaster.pid exactly,
 # so they may mistake a pre-existing postmaster.pid for one created by the
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index fe09689..a2df156 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -37,6 +37,7 @@ our @EXPORT = qw(
   program_version_ok
   program_options_handling_ok
   command_like
+  command_like_safe
   command_fails_like
 
   $windows_os
@@ -300,6 +301,23 @@ sub command_like
 	like($stdout, $expected_stdout, "$test_name: matches");
 }
 
+sub command_like_safe
+{
+	# Doesn't rely on on detecting end of file on the file descriptors,
+	# which can fail, causing the process to hang, notably on Msys
+	# when used with 'pg_ctl start'
+	my ($cmd, $expected_stdout, $test_name) = @_;
+	my ($stdout, $stderr);
+	print("# Running: " . join(" ", @{$cmd}) . "\n");
+	my $result = IPC::Run::run $cmd, '>', 'stdout.txt', '2>', 'stderr.txt';
+	$stdout = slurp_file('stdout.txt');
+	$stderr = slurfp_file('stderr.txt');
+	unlink 'stdout.txt','stderr.txt';
+	ok($result, "$test_name: exit code 0");
+	is($stderr, '', "$test_name: no stderr");
+	like($stdout, $expected_stdout, "$test_name: matches");
+}
+
 sub command_fails_like
 {
 	my ($cmd, $expected_stderr, $test_name) = @_;

-- 
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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/25/2017 01:41 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 07/25/2017 11:25 AM, Tom Lane wrote:
>>> Oh.  So when was the last time it worked?  And why do the buildfarm
>>> archives contain apparently-successful log_stage entries for pg_ctl-check
>>> on jacana, up through yesterday when I looked?
>> There was a buildfarm bug, since corrected, which was not properly
>> picking up errors in a corner case. You will see the errors if you look
>> at all the logs for pg_ctl-check from 12 days back if they exist. The
>> last known actual good run of this test was 33 days ago, i.e. 2017-06-22
> Hm.  This failure presumably started with commit f13ea95f9, which
> introduced use of command_like() in the pg_ctl TAP test; but that
> didn't go in until 2017-06-28.  Was jacana not running in the
> interim?


Looks like not, possibly because one of the other machines on this box
was hung (probably lorikeet).


>
> Anyway, if we believe that it broke with f13ea95f9, hen it's plausible
> that CMD.EXE has been sharing pg_ctl's stdout/stderr all along, and we
> just had not noticed before.  Could you check what happens if we
> change the bInheritHandles parameter as I suggested upthread?
>
>       


I'll try when I get all the animals caught up.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/25/2017 11:32 AM, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> On Tue, Jul 25, 2017 at 11:00 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Hm, I had the idea that we were already asking ExtUtils::Embed for that,
>>> but now I see we only inquire about LDFLAGS not CCFLAGS.  Yes, this sounds
>>> like a promising avenue to pursue.
>>>
>>> It would be useful to see the results of
>>> perl -MExtUtils::Embed -e ccopts
>>> on one of the affected installations, and compare that to the problematic
>>> field(s).
>> Why ccopts rather than ccflags?
> I was looking at the current code which fetches ldopts, and analogizing.
> Don't know the difference between ccflags and ccopts.
>
>   


per docs:

ccopts()
This function combines "perl_inc()", "ccflags()" and
"ccdlflags()"
into one.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/25/2017 11:25 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 07/25/2017 11:11 AM, Tom Lane wrote:
>>> What I'm on about is that jacana still succeeds entirely, more than half
>>> the time.  If this is a handle-duplication problem, how could it ever
>>> succeed?
>> No, it hangs 100% of the time. The only time you see a result at all is
>> if I manually intervene. The pg_ctl test is thus currently disabled  on
>> jacana.
> Oh.  So when was the last time it worked?  And why do the buildfarm
> archives contain apparently-successful log_stage entries for pg_ctl-check
> on jacana, up through yesterday when I looked?
>
>   


There was a buildfarm bug, since corrected, which was not properly
picking up errors in a corner case. You will see the errors if you look
at all the logs for pg_ctl-check from 12 days back if they exist. The
last known actual good run of this test was 33 days ago, i.e. 2017-06-22

cheers

andrew



-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/25/2017 11:11 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 07/24/2017 09:33 PM, Tom Lane wrote:
>>> Seems like the TAP test should fail every time, if this is a full
>>> description.  But maybe it's part of the answer, so it seems worth
>>> experimenting in this direction.
>> The test that hangs is the only case where we call pg_ctl via
>> command_like. If we use other mechanisms such as command_ok that don't
>> try to read the output there is no problem.
> What I'm on about is that jacana still succeeds entirely, more than half
> the time.  If this is a handle-duplication problem, how could it ever
> succeed?
>



No, it hangs 100% of the time. The only time you see a result at all is
if I manually intervene. The pg_ctl test is thus currently disabled  on
jacana.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/25/2017 11:00 AM, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> Perl also has a mechanism for flags added to Configure to be passed
>> along when building loadable modules; if it didn't, not just plperl
>> but every Perl module written in C would have this issue if any such
>> flags where used.
>> ...
>> While I'm not sure of the details, I suspect that we need to use one
>> of those methods to get the CCFLAGS used to build perl, and include
>> those when SPI.o, Util.o, and plperl.o in src/pl/plperl.  Or at least
>> the -D switches from those CCFLAGS.
> Hm, I had the idea that we were already asking ExtUtils::Embed for that,
> but now I see we only inquire about LDFLAGS not CCFLAGS.  Yes, this sounds
> like a promising avenue to pursue.
>
> It would be useful to see the results of
>
> perl -MExtUtils::Embed -e ccopts
>
> on one of the affected installations, and compare that to the problematic
> field(s).

  -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE -DPERL_TEXTMODE_SCRIPTS
-DUSE_SITECUSTOMIZE -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -fwrapv
-fno-strict-aliasing -mms-bitfields  -I"C:\Perl64\lib\CORE"

cheers

andrew

-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

On 07/24/2017 09:33 PM, Tom Lane wrote:
>
> [good theory about why pg_ctl hangs in TAP test]
>
> Now, this theory still has a Mack-truck-sized hole in it, which is
> if that's the failure mechanism then why isn't it 100% effective?
> Seems like the TAP test should fail every time, if this is a full
> description.  But maybe it's part of the answer, so it seems worth
> experimenting in this direction.


The test that hangs is the only case where we call pg_ctl via
command_like. If we use other mechanisms such as command_ok that don't
try to read the output there is no problem.

Another data point is that this test doesn't hang on bowerbird, which is
an animal on the same machine but doing an MSVC build. Thus my thesis
that it's probably to do with the imteraction with the MSys perl and shell.

A simple workaround might be to provide two flavors of command_like, one
that uses files it then slurps in and one that uses direct scalars and
EOF detection.

cheers

andrew


>
> A bit of googling Microsoft's documentation suggests that maybe all
> we have to do is pass CreateProcessAsUser's bInheritHandles parameter
> as FALSE not TRUE in pg_ctl.c.  It's not apparent to me that there
> are any handles we do need the CMD process to inherit.
>
>       


Maybe.

cheers

andrew


-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/25/2017 08:58 AM, Amit Kapila wrote:
>
> I think the real question is where do we go from here.  Ashutosh has
> proposed a patch up-thread based on a suggestion from Andrew, but it
> is not clear if we want to go there as that seems to be bypassing
> handshake mechanism.  The other tests and analysis seem to indicate
> that the new version Perl binaries on Windows are getting built with
> flags that are incompatible with what we use for plperl and it is not
> clear why perl is using those flags.  Do you think we can do something
> at our end to make it work or someone should check with Perl community
> about it?
>


No amount of checking with the Perl community is likely to resolve this
quickly w.r.t. existing releases of Perl.

We seem to have a choice either to abandon, at least temporarily, perl
support on Windows for versions of perl >= 5.20 (I think) or adopt the
suggestion I made. It's not a complete hack - it's something at least
somewhat blessed in perl's XSUB.h:

/* dXSBOOTARGSNOVERCHK has no API in xsubpp to choose it so do
#undef dXSBOOTARGSXSAPIVERCHK
#define dXSBOOTARGSXSAPIVERCHK dXSBOOTARGSNOVERCHK */


Note that on earlier versions of perl we got by without this check for
years without any issue or complaint I recall hearing of. If we don't
adopt this I would not be at all surprised to see Windows packagers
adopt it anyway.

cheers

andrew



-- 
Andrew Dunstanhttps://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] Testlib.pm vs msys

[Andrew Dunstan]

It turns out I was wrong about the problem jacana has been having with
the pg_ctl tests hanging. The problem was not the use of select as a
timeout mechanism, although I think the change to using
Time::Hires::usleep() is correct and shouldn't be reverted.

The problem is command_like's use of redirection to strings. Why this
should be a problem for this particular use is a matter of speculation.
I suspect it's to do with the fact that in this instance pg_ctl is
leaving behind some child processes (i.e. postmaster and children) after
it exits, and so on this particular path IPC::Run isn't detecting the
exit properly. The workaround I have found to work is to redirect
command_like's output instead to a couple of files and then slurp in
those files and delete them. A bit hacky, I know, so I'm open to other
suggestions.


cheers


andrew

-- 
Andrew Dunstanhttps://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] Syncing sql extension versions with shared library versions

[Andrew Dunstan]

On 07/21/2017 04:17 PM, Mat Arye wrote:
> Hi All,
>
> I am developing the TimescaleDB extension for postgres
> (https://github.com/timescale/timescaledb) and have some questions
> about versioning. First of all, I have to say that the versioning
> system on the sql side is wonderful. It's really simple to write
> migrations etc.
>
> However when thinking through the implications of having a database
> cluster with databases having different extension versions installed,
> it was not apparently clear to me how to synchronize the installed
> extension version with a shared library version. For example, if I
> have timescaledb version 0.1.0 in one db and version 0.2.0 in another
> db, I'd like for timescaledb-0.1.0.so <http://timescaledb-0.1.0.so>
> and  timescaledb-0.2.0.so <http://timescaledb-0.2.0.so> to be used,
> respectively. (I want to avoid having to keep backwards-compatibility
> for all functions in future shared-libraries). In our case, this is
> further complicated by the fact that we need to preload the shared
> library since we are accessing the planner hooks etc. Below, I'll
> describe some solutions I have been thinking about, but wanted to hear
> if anyone else on this list has already solved this problem and has
> some insight. It is also quite possible I am missing something. 
>
> Issue 1: Preloading the right shared library.
> When preloading libraries (either via local_preload_libraries, or
> session_preload_libraries, shared_preload_libraries), it would be nice
> to preload the shared_library according to it's version. But, I looked
> through the code and found no logic for adding version numbers to
> shared library names.
> Solution 1: Set session_preload_libraries on the database via ALTER
> DATABASE SET. This can be done in the sql and the sql
> version-migration scripts can change this value as you change
> extensions versions. I think this would work, but it seems very
> hack-ish and less-than-ideal.
> Solution 2: Create some kind of stub shared-library that will, in
> turn, load another shared library of the correct version. This seems
> like the cleaner approach. Has anybody seen/implemented something like
> this already?
>
> Issue 2: module_pathname
> I believe that for user defined function the MODULE_PATHNAME
> substitution will not work since that setting is set once
> per-extension. Thus, for example, the migration scripts that include
> function definitions for older versions would use the latest .so file
> if MODULE_PATHNAME was used in the definition. This would be a problem
> if upgrading to an intermediate (not latest) version.
> Solution: MODULE_PATHNAME cannot be used, and we should build our own
> templating/makefile infrastructure to link files to the
> right-versioned shared library in the CREATE FUNCTION definition. 
>
>



It would be nice if we could teach yhe load mechanism to expand a a
version escape in the MODULE_PATHNAME. e.g.

MODULE_PATHNAME = '$libdir/foo-$version'

cheers

andtrew



-- 
Andrew Dunstanhttps://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] Unportable use of select for timeouts in PostgresNode.pm

[Andrew Dunstan]

I've been trying to get to the bottom of a nasty hang in buildfarm
member jacana when running the pg_ctl TAP test. This test used to work,
and was last known to work on June 22nd.

My attention has become focussed on this change in commit de3de0afd:

-   # Wait a second before retrying.
-   sleep 1;
+   # Wait 0.1 second before retrying.
+   select undef, undef, undef, 0.1;

This is a usage that is known not to work in Windows - IIRC we
eliminated such calls from our C programs at the time of the Windows
port - and it seems to me very likely to be the cause of the hang.
Instead I think we should use the usleep() function from the standard
(from 5.8) Perl module Time::HiRes, as recommended in the Perl docs for
the sleep() function for situations where you need finer grained
timeouts. I have verified that this works on jacana and friends.

Unless I hear objections I'll prepare a patch along those lines.


cheers


andrew

-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/13/2017 10:36 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 07/13/2017 08:08 AM, Ashutosh Sharma wrote:
>>> -dVAR; dXSBOOTARGSAPIVERCHK;
>>> +dVAR; dXSBOOTARGSNOVERCHK;
>> Good job hunting this down!
>> One suggestion I saw in a little googling was that we add this to the XS
>> file after the inclusion of XSUB.h:
>> #undef dXSBOOTARGSAPIVERCHK
>> #define dXSBOOTARGSAPIVERCHK dXSBOOTARGSNOVERCHK
> I don't see anything even vaguely like that in the Util.c file generated
> by Perl 5.10.1, which is what I've got on my RHEL machine.




This is all fairly modern, so it's hardly surprising that it doesn't
happen with the ancient perl 5.10.

here's a snippet from the generated Util.c on crake (Fedora 25, perl 5.24):

XS_EXTERNAL(boot_PostgreSQL__InServer__Util); /* prototype to pass
-Wmissing-prototypes */
XS_EXTERNAL(boot_PostgreSQL__InServer__Util)
{
#if PERL_VERSION_LE(5, 21, 5)
dVAR; dXSARGS;
#else
dVAR; dXSBOOTARGSAPIVERCHK;
#endif



>
> What I do notice is this in Util.xs:
>
> VERSIONCHECK: DISABLE
>
> which leads immediately to two questions:
>
> 1. Why is your version of xsubpp apparently ignoring this directive
> and generating a version check anyway?
>
> 2. Why do we have this directive in the first place?  It does not seem
> to me like a terribly great idea to ignore low-level version mismatches.
>
> In the same vein, I'm suspicious of proposals to "fix" this problem
> by removing the version check, which seems to be where Ashutosh
> is headed.  In the long run that seems certain to cause huge headaches.



That is a different version check. It's the equivalent of xsubpp's
--noversioncheck flag. The versions it would check are the object file
and the corresponding pm file.

In fact the usage I suggested seems to be blessed in XSUB.h in this comment:

/* dXSBOOTARGSNOVERCHK has no API in xsubpp to choose it so do
#undef dXSBOOTARGSXSAPIVERCHK
#define dXSBOOTARGSXSAPIVERCHK dXSBOOTARGSNOVERCHK */





It would be nice to get to the bottom of why we're getting a version
mismatch on Windows, since we're clearly not getting one on Linux. But
since we've got on happily all these years without the API version check
we might well survive a few more going on as we are.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/13/2017 08:08 AM, Ashutosh Sharma wrote:
>
> After doing some study, I could understand that Util.c is generated
> from Util.xs by xsubpp compiler at build time. This is being done in
> Mkvcbuild.pm file in postgres. If I manually replace
> 'dXSBOOTARGSAPIVERCHK' macro with 'dXSBOOTARGSNOVERCHK' macro in
> src/pl/plperl/Util.c, the things work fine. The diff is as follows,
>
> XS_EXTERNAL(boot_PostgreSQL__InServer__Util)
> {
> #if PERL_VERSION_LE(5, 21, 5)
> dVAR; dXSARGS;
> #else
> -dVAR; dXSBOOTARGSAPIVERCHK;
> +dVAR; dXSBOOTARGSNOVERCHK;
>
> I need to further investigate, but let me know if you have any ideas.




Good job hunting this down!


One suggestion I saw in a little googling was that we add this to the XS
file after the inclusion of XSUB.h:

#undef dXSBOOTARGSAPIVERCHK
#define dXSBOOTARGSAPIVERCHK dXSBOOTARGSNOVERCHK

cheers

andrew

-- 
Andrew Dunstanhttps://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] pl/perl extension fails on Windows

[Andrew Dunstan]

On 07/12/2017 11:49 AM, Dave Page wrote:
>
>
> On Wed, Jul 12, 2017 at 4:35 PM, Tom Lane <t...@sss.pgh.pa.us
> <mailto:t...@sss.pgh.pa.us>> wrote:
>
> Sandeep Thakkar <sandeep.thak...@enterprisedb.com
> <mailto:sandeep.thak...@enterprisedb.com>> writes:
> > I compiled PG 10 beta1/beta2 with "--with-perl" option on
> Windows and the
> > extension crashes the database.
> > *src/pl/plperl/Util.c: loadable library and perl binaries are
> mismatched
> > (got handshake key 0A900080, needed 0AC80080)*
>
> > This is seen with Perl 5.24 but not with 5.20, 5.16. What I
> found is that
> > the handshake function is added in Perl 5.21.x and probably that
> is why we
> > don't see this issue in earlier versions.
>
> Well, we have various buildfarm machines running perls newer than
> that,
> eg, crake, with 5.24.1.  So I'd say there is something busted
> about your
> perl installation.  Perhaps leftover bits of an older version
> somewhere?
>
>
> Well crake is a Fedora box - and we have no problems on Linux, only on
> Windows. 
>
>


Yeah, I have this on one of my Windows boxes, and haven't had time to
get to the bottom of it yet ;-(

Latest versions of ActivePerl don't ship with library descriptor files,
either, which is unpleasant.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Arrays of domains

[Andrew Dunstan]

On 07/11/2017 12:44 PM, Tom Lane wrote:
>
> Can anyone think of a reason not to pursue that?
>
>   


I'm all for it.

cheers

andrew

-- 
Andrew Dunstanhttps://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] intermittent failures in Cygwin from select_parallel tests

[Andrew Dunstan]

On 06/26/2017 10:45 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 06/23/2017 07:47 AM, Andrew Dunstan wrote:
>>> Rerunning with some different settings to see if I can get separate cores.
>> Numerous attempts to get core dumps following methods suggested in
>> Google searches have failed. The latest one is just hanging.
> Well, if it's hung, maybe you could attach to the processes with gdb and
> get stack traces manually?
>
>   



In theory what I have set up is supposed to be doing that, but I'll try.

cheers

andrew

-- 
Andrew Dunstanhttps://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] intermittent failures in Cygwin from select_parallel tests

[Andrew Dunstan]

On 06/26/2017 10:36 AM, Amit Kapila wrote:
> On Fri, Jun 23, 2017 at 9:12 AM, Andrew Dunstan
> <andrew.duns...@2ndquadrant.com> wrote:
>>
>> On 06/22/2017 10:24 AM, Tom Lane wrote:
>>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>>> Please let me know if there are tests I can run. I  missed your earlier
>>>> request in this thread, sorry about that.
>>> That earlier request is still valid.  Also, if you can reproduce the
>>> symptom that lorikeet just showed and get a stack trace from the
>>> (hypothetical) postmaster core dump, that would be hugely valuable.
>>>
>>>
>>
>> See attached log and stacktrace
>>
> Is this all the log contents or is there something else?  The attached
> log looks strange to me in the sense that the first worker that gets
> invoked is Parallel worker number 2 and it finds that somebody else
> has already set the sender handle.
>



No, it's the end of the log file. I can rerun and get the whole log file
if you like.

cheers

andrew

-- 
Andrew Dunstanhttps://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] intermittent failures in Cygwin from select_parallel tests

[Andrew Dunstan]

On 06/23/2017 07:47 AM, Andrew Dunstan wrote:
>
> On 06/23/2017 12:11 AM, Tom Lane wrote:
>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>> On 06/22/2017 10:24 AM, Tom Lane wrote:
>>>> That earlier request is still valid.  Also, if you can reproduce the
>>>> symptom that lorikeet just showed and get a stack trace from the
>>>> (hypothetical) postmaster core dump, that would be hugely valuable.
>>> See attached log and stacktrace
>> The stacktrace seems to be from the parallel-query-leader backend.
>> Was there another core file?
>>
>> The lack of any indication in the postmaster log that the postmaster saw
>> the parallel worker's crash sure looks the same as lorikeet's failure.
>> But if the postmaster didn't dump core, then we're still at a loss as to
>> why it didn't respond.
>>
>>  
>
>
> Rerunning with some different settings to see if I can get separate cores.
>


Numerous attempts to get core dumps following methods suggested in
Google searches have failed. The latest one is just hanging.

cheers

andrew


-- 
Andrew Dunstanhttps://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] intermittent failures in Cygwin from select_parallel tests

[Andrew Dunstan]

On 06/23/2017 12:11 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 06/22/2017 10:24 AM, Tom Lane wrote:
>>> That earlier request is still valid.  Also, if you can reproduce the
>>> symptom that lorikeet just showed and get a stack trace from the
>>> (hypothetical) postmaster core dump, that would be hugely valuable.
>> See attached log and stacktrace
> The stacktrace seems to be from the parallel-query-leader backend.
> Was there another core file?
>
> The lack of any indication in the postmaster log that the postmaster saw
> the parallel worker's crash sure looks the same as lorikeet's failure.
> But if the postmaster didn't dump core, then we're still at a loss as to
> why it didn't respond.
>
>   



Rerunning with some different settings to see if I can get separate cores.

cheers

andrew


-- 
Andrew Dunstanhttps://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] intermittent failures in Cygwin from select_parallel tests

[Andrew Dunstan]

On 06/22/2017 10:24 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> Please let me know if there are tests I can run. I  missed your earlier
>> request in this thread, sorry about that.
> That earlier request is still valid.  Also, if you can reproduce the
> symptom that lorikeet just showed and get a stack trace from the
> (hypothetical) postmaster core dump, that would be hugely valuable.
>
>   


See attached log and stacktrace

cheers

andrew

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

??
??:0
??
??:0
??
??:0
??
??:0
??
??:0
??
??:0
errfinish
/home/andrew/bf64/root/HEAD/pgsql/src/backend/utils/error/elog.c:555
elog_finish
/home/andrew/bf64/root/HEAD/pgsql/src/backend/utils/error/elog.c:1378
s_lock_stuck
/home/andrew/bf64/root/HEAD/pgsql/src/backend/storage/lmgr/s_lock.c:83
s_lock
/home/andrew/bf64/root/HEAD/pgsql/src/backend/storage/lmgr/s_lock.c:98
shm_mq_detach
/home/andrew/bf64/root/HEAD/pgsql/src/backend/storage/ipc/shm_mq.c:783
slist_is_empty
/home/andrew/bf64/root/HEAD/pgsql/src/include/lib/ilist.h:567
DestroyParallelContext
/home/andrew/bf64/root/HEAD/pgsql/src/backend/access/transam/parallel.c:629
ExecParallelCleanup
/home/andrew/bf64/root/HEAD/pgsql/src/backend/executor/execParallel.c:672
ExecShutdownGather
/home/andrew/bf64/root/HEAD/pgsql/src/backend/executor/nodeGather.c:408


pglog
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] intermittent failures in Cygwin from select_parallel tests

[Andrew Dunstan]

On 06/21/2017 06:44 PM, Tom Lane wrote:
> Today, lorikeet failed with a new variant on the bgworker start crash:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet=2017-06-21%2020%3A29%3A10
>
> This one is even more exciting than the last one, because it sure looks
> like the crashing bgworker took the postmaster down with it.  That is
> Not Supposed To Happen.
>
> Wondering if we broke something here recently, I tried to reproduce it
> on a Linux machine by adding a randomized Assert failure in
> shm_mq_set_sender.  I don't see any such problem, even with EXEC_BACKEND;
> we recover from the crash as-expected.
>
> So I'm starting to get a distinct feeling that there's something wrong
> with the cygwin port.  But I dunno what.
>
>   


:-(

Please let me know if there are tests I can run. I  missed your earlier
request in this thread, sorry about that.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pg_bsd_indent 2.0 is available from git.postgresql.org

[Andrew Dunstan]

On 06/21/2017 02:25 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 06/21/2017 11:30 AM, Tom Lane wrote:
>>> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>>>> Unfortunately this seems precluded by the use of the non-standard
>>>> "err.h". It looks like that will trip us up on mingw also.
>>> Hm, why?  Doesn't the -I search path get the right thing?
>> The file isn't present at all.
> Uh, what?  It's in the repo.
>
>       


Oh, sorry, my bad, brain fade while multitasking.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pg_bsd_indent 2.0 is available from git.postgresql.org

[Andrew Dunstan]

On 06/21/2017 11:30 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> Unfortunately this seems precluded by the use of the non-standard
>> "err.h". It looks like that will trip us up on mingw also.
> Hm, why?  Doesn't the -I search path get the right thing?
>
>   


The file isn't present at all. On my Linux workstation "man errx" says:

CONFORMING TO
   These functions are nonstandard BSD extensions.


cheers

andrew


-- 
Andrew Dunstanhttps://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] pg_bsd_indent 2.0 is available from git.postgresql.org

[Andrew Dunstan]

On 06/21/2017 08:20 AM, Andrew Dunstan wrote:
>
> On 06/20/2017 08:30 PM, Tom Lane wrote:
>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>> On Wed, Jun 21, 2017 at 8:43 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>> Yeah, I thought it would work fine with Makefile-using Windows toolchains.
>>>> But people who use MSVC need something else, no?
>>> Are there that many anyway who care?
>> Well, *I* don't care, but I thought we wanted to support Windows-using
>> developers as reasonably first-class citizens.
>>
>>  
>
>
> I imagine we can rig up something fairly simple based on Craig Ringer's
> recent work in building extensions on Windows using cmake.
>
> I'll take a look when I get a chance, but I don't think it's a high
> priority.
>


Unfortunately this seems precluded by the use of the non-standard
"err.h". It looks like that will trip us up on mingw also.

cheers

andrew



-- 
Andrew Dunstanhttps://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] pg_bsd_indent 2.0 is available from git.postgresql.org

[Andrew Dunstan]

On 06/20/2017 08:30 PM, Tom Lane wrote:
> Michael Paquier <michael.paqu...@gmail.com> writes:
>> On Wed, Jun 21, 2017 at 8:43 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Yeah, I thought it would work fine with Makefile-using Windows toolchains.
>>> But people who use MSVC need something else, no?
>> Are there that many anyway who care?
> Well, *I* don't care, but I thought we wanted to support Windows-using
> developers as reasonably first-class citizens.
>
>   



I imagine we can rig up something fairly simple based on Craig Ringer's
recent work in building extensions on Windows using cmake.

I'll take a look when I get a chance, but I don't think it's a high
priority.

cheers

andrew

-- 
Andrew Dunstanhttps://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] pg_bsd_indent 2.0 is available from git.postgresql.org

[Andrew Dunstan]

On 06/20/2017 04:17 PM, Tom Lane wrote:
> I've set up a repo on our git server for the new improved version
> of pg_bsd_indent.  Please test it for portability to your favorite
> platform(s) by doing
>
> git clone https://git.postgresql.org/git/pg_bsd_indent.git
> cd pg_bsd_indent
> make -s all
> make check
>
> Note you will need a PG installation in your PATH; see
> README.pg_bsd_indent for details.
>
> There's no Windows support yet, and I won't be writing that,
> but I hope someone else will.


What extra support do you think it needs? I have built it on Cygwin
without touching a line of code, will probably be able to to the same on
Mingw.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Announcing Release 5 of the PostgreSQL Buildfarm Client

[Andrew Dunstan]

Release 5 of the PostgreSQL Buildfarm Client has been released and can
be downloaded from



In a similar move to PostgreSQL version numbering, with this release we
move to a one part numbering system.


In addition to a number of bug fixes and very minor changes, this
release has the following features / changes:


  * Cross-version pg_upgrade module is no longer experimental - see below
  * TAP tests now run the "check" target, but in most cases redundant
installs are avoided
  * Improved and expanded TAP test running on MSVC animals - these now
run the same tests as other animals
  * Automatic garbage collection of git repositories, once a week by
default. This should improve the speed of git operations, especially
on older git releases.
  * Improve logging in SEpgsql module
  * Revert 4.19 setting of TZ by default. If you want it set then do so
in the config file. Doing so can speed up initdb. Setting it by
default caused too many anomalies.
  * The environment setting BF_LOG_TIME (settable in the config file)
causes command output to be prefixed with a timestamp on Unix
platforms if /usr/bin/ts is present. 


The cross-version upgrade module (TestUpgradeXVersion) has long been an
experimental module. It has been refactored and set to report its
results properly to the server, so it's now ready for wider use. Its use
requires quite a lot of disk space, so it's not configured by default.
You need to have at least 5Gb of available space in order to use it.
Normally in a buildfarm run all the build products are removed. This
module saves them in a special area so that they are available for
testing against different branches. Each branch is tested against itself
and any earlier branches that the module has saved. So currently for the
HEAD (i.e. master) branch it can test upgrades from 9.2, 9.3, 9.4, 9.5
and 9.6 as well as itself. The tests cover all the databases in the
install, such as each contrib database, not just the standard regression
database. The module has a heuristic test for success. Once the upgrade
has run successfully, we compare a pre-upgrade dump with a post-upgrade
dump. If it's a same branch upgrade we expect 0 lines of difference, but
for a cross-branch upgrade we allow up to 2000 lines of difference.
Clearly this isn't a perfect test, but experience with the module over
an extended period has shown that pretty much all problems either result
in an upgrade failure or a diff that is larger than 2000. See

for an example of output in upgrading REL9_2_STABLE to HEAD. The tests
take about 60 to 90 seconds per test on my test machine - so in a full
buildfarm run across all live branches (21 upgrade tests in all) that
adds about 30 minutes.

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

Re: [HACKERS] Buildfarm failures on woodlouse (in ecpg-check)

[Andrew Dunstan]

On 06/11/2017 11:33 AM, Christian Ullrich wrote:
> Hello,
>
> my buildfarm animal woodlouse (Visual Studio 2013 on Windows 7)
> stopped working correctly some months ago. After Tom kindly prodded me
> into fixing it, I noticed that I had configured it to skip the
> ecpg-check step because one of the tests in the "thread" section (not
> always the same) nearly always failed.
>
> I had set it up in circa 2015, so I reenabled the step to see whether
> anything had changed since, and it started failing again.
>
> Through some trial and error, and with the help of Microsoft's
> Application Verifier tool, I found what I think is the cause: It looks
> like the VS 2013 CRT's setlocale() function is not entirely
> thread-safe; the heap debugging options make it crash when
> manipulating per-thread locale state, and according to the comments
> surrounding that spot in the CRT source, the developers were aware the
> code is fragile.
>
> I crudely hacked a critical section around the setlocale() call in
> pgwin32_setlocale(). After this change, the test does not crash, while
> without it, it crashes every time.
>
> If there is interest in fixing this issue that is apparently limited
> to VS 2013, I will try and produce a proper patch. I notice, however,
> that there is a pthread compatibility layer available that I have no
> experience with at all, and I assume using it is preferred over
> straight Win32?
>
> My hack is attached; please feel free to tell me I'm on the wrong track.
> To build correctly, it requires defining _WIN32_WINNT to be 0x600 or
> above (and using an SDK that knows about InitOnceExecuteOnce()).
>
>

It's certainly worth doing.

I turned off testing ecpg ages ago on bowerbird because I was getting
errors. That's an older version of the toolset.

We already define _WIN32_WINNT to be 0x0600 on all appropriate platforms
(Vista/2008 and above), so I think you could probably just check for
that value.

I have no opinion on the pthread question.

cheers

andrew

-- 
Andrew Dunstanhttps://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] jsonb_to_tsvector should be immutable

[Andrew Dunstan]

On 06/08/2017 03:06 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 06/08/2017 02:26 PM, Tom Lane wrote:
>>> Yeah, if the (regconfig,text) one is considered immutable, I don't see
>>> why the other two aren't.  The justification for the other three being
>>> only stable is that they depend on default_text_search_config.
>> Yes, agreed it should be done consistently with text.
> You going to fix it, or shall I?
>
>   


I'll do it.

-- 
Andrew Dunstanhttps://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] jsonb_to_tsvector should be immutable

[Andrew Dunstan]

On 06/08/2017 02:26 PM, Tom Lane wrote:
> Josh Berkus <j...@berkus.org> writes:
>> select proname, prosrc, proargtypes, provolatile from pg_proc where
>> proname = 'to_tsvector';
> Slightly more readable version:
>
> regression=# select oid::regprocedure, provolatile, proparallel from pg_proc 
> where proname = 'to_tsvector';
>  oid  | provolatile | proparallel 
> --+-+-
>  to_tsvector(jsonb)   | s   | s
>  to_tsvector(regconfig,text)  | i   | s
>  to_tsvector(text)| s   | s
>  to_tsvector(json)| s   | s
>  to_tsvector(regconfig,jsonb) | s   | s
>  to_tsvector(regconfig,json)  | s   | s
> (6 rows)
>
>> Both of the _byid functions should be marked immutable, no?  Otherwise
>> how can users use the new functions for indexing?
> Yeah, if the (regconfig,text) one is considered immutable, I don't see
> why the other two aren't.  The justification for the other three being
> only stable is that they depend on default_text_search_config.
>
> (You could argue that none of these should be immutable because text
> search configurations are changeable, but we already decided to ignore
> that for the (regconfig,text) case.)
>
>       



Yes, agreed it should be done consistently with text.

cheers

andrew


-- 
Andrew Dunstanhttps://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] Does pg_upgrade really support "make installcheck"?

[Andrew Dunstan]

On 06/08/2017 03:04 AM, Neha Khatri wrote:
> On 6/7/17, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> src/bin/pg_upgrade/TESTING claims (much further down in the file
>> than I'd like):
>>
>>  The shell script test.sh in this directory performs more or less this
>>  procedure.  You can invoke it by running
>>  make check
>>  or by running
>>  make installcheck
>>  if "make install" (or "make install-world") were done beforehand.
>>
>> However, the second alternative doesn't really work:
>>
>> $ make installcheck
>> make: Nothing to be done for `installcheck'.
>> $
>>
>> Did this ever work, or could it easily be made to work?
> It seems it would work if the following two lines are uncommented from
> the src/bin/pg_upgrade/Makefile:
>
>   # disabled because it upsets the build farm
>   # installcheck: test.sh
>   #  MAKE=$(MAKE) bindir=$(bindir) libdir=$(libdir) $(SHELL) $<
>
> As the comment says, it was purposely disabled, probably to avoid
> failure on cetain build farm members.
> Attached the result of make installcheck after enabling the
> intallcheck target (just to be sure if that is what you are looking
> for).
>
>> If not, we need to fix that documentation.
> If the attached is result is what you are after, should the
> documentation be updated to mention that make installcheck is
> purposely disabled, providing the reason for it. Or, should the
> intallcheck target be enabled in the Makefile to find out if specific
> buildfarm members still complain about it.



The whole thing is explained here:
<https://www.postgresql.org/message-id/20327.1322435...@sss.pgh.pa.us>

And since then the buildfarm has acquired a separate optional module
that tests pg_upgrade. It's enabled by default in the sample config file.

So it's not like we don't have buildfarm coverage - we do in fact.
Re-enabling this in the Makefile would a) result in breakage on some
members and b) make most members do redundant work.

I vote for improving the docs.

Let's also note that this test is not great anyway for a couple of
reasons. First, it doesn't test as much as it might, only the core
regression database. And second (and probably more importantly) it
doesn't test cross-version upgrade, which, after all, is the reason for
pg_upgrade's existence. There is an experimental buildfarm module that
addresses both these issues, but it needs a bit of work to make it
production ready. It runs (without reporting) on the animal crake, and
has been stable since some time in April. I recently started work on
bringing it up to production standard. It does take up a lot of space,
since it's saving out binaries and databases that are normally removed
at the end of each buildfarm run. On crake the static space required is
3.2Gb. That's in contrast to roughly 0.5 Gb used for all the supported
branches for everything else.


cheers

andrew

-- 
Andrew Dunstanhttps://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] PROVE_FLAGS

[Andrew Dunstan]

On 05/23/2017 06:59 AM, Andrew Dunstan wrote:
> On 17 May 2017 at 14:30, Robert Haas <robertmh...@gmail.com> wrote:
>> On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan
>> <andrew.duns...@2ndquadrant.com> wrote:
>>> Inheriting variables from the environment is a part of make by design.
>>> We have PG_PROVE_FLAGS for our own forced settings.
>> I don't buy this argument.  We've had previous cases where we've gone
>> through and added -X to psql invocations in the regression tests
>> because, if you don't, some people's .psqlrc files may cause tests to
>> fail, which nobody wants.  The more general principle is that the
>> regression tests should ideally pass regardless of the local machine
>> configuration.  It's proven impractical to make that universally true,
>> but that doesn't make it a bad goal.  Now, for all I know it may be
>> that setting PROVE_FLAGS can never cause a regression failure, but I
>> still tend to agree with Peter that the regression tests framework
>> should insulate us from the surrounding environment rather than
>> absorbing settings from it.
>>
> In that case we're going to need to invent a way to do this similarly
> in vcregress.pl. I'm not simply going to revert to the situation where
> it and the makefiles are completely out of sync on this. The previous
> patch was made more or less by ignoring the existence of vcregress.pl.
>
> I will look at this again probably after pgcon. I don't think it's
> terribly urgent.
>


Here's a patch that should do that. If this is acceptable I'll do this
and put the makefiles back the way they were.

cheers

andrew

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

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 468a62d..e2d225e 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -178,12 +178,18 @@ sub tap_check
 	die "Tap tests not enabled in configuration"
 	  unless $config->{tap_tests};
 
+	my @flags;
+	foreach my $arg (0 .. scalar(@_))
+	{
+		next unless $_[$arg] =~ /^PROVE_FLAGS=(.*)/;
+		@flags = split(/\s+/, $1};
+		splice(@_,$arg,1);
+		last;
+	}
+
 	my $dir = shift;
 	chdir $dir;
 
-	my @flags;
-	@flags = split(/\s+/, $ENV{PROVE_FLAGS}) if exists $ENV{PROVE_FLAGS};
-
 	my @args = ("prove", @flags, "t/*.pl");
 
 	# adjust the environment for just this test

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