Re: set TESTDIR from perl rather than Makefile

[Andrew Dunstan]

On 2/19/22 18:53, Justin Pryzby wrote:
> On Sat, Feb 19, 2022 at 05:41:49PM -0600, Justin Pryzby wrote:
>> I rebased and fixed the check-guc script to work, made it work with vpath
>> builds, and cleaned it up some.
> I also meant to also attach it.


This is going to break a bunch of stuff as written.

First, it's not doing the same thing. The current system sets TESTDIR to
be the parent of the directory that holds the test. e.g. for
src/bin/pg_ctl/t/001_start_stop.pl it's src/bin/pg_ctl in the build
tree, not the 't' subdirectory. This patch apparently sets it to the 't'
subdirectory. That will break anything that goes looking for log files
in the current location, like the buildfarm client, and possibly some CI
setups as well.

Also, unless I'm mistaken it appears to to the wrong thing for vpath
builds:

my $test_dir = File::Spec->rel2abs(dirname($0));

is completely wrong for vpaths, since that will place it in the source
tree, not the build tree.

Last, and for no explained reason that I can see, the patch undoes
commit f4ce6c4d3a, but only for msvc builds. Even if that's justified it
appears to have no relevance to this patch.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Using Test::More test functions for pg_rewind

[Andrew Dunstan]

On 2/21/22 09:10, Daniel Gustafsson wrote:
> check_query() in RewindTest.pm currently has this comment before handrolling
> tests for return code and stderr:
>
>   # We don't use ok() for the exit code and stderr, because we want this
>   # check to be just a single test.
>
> The code came with the initial import of pg_rewind, and there is no further
> explanation but I guess it was to make test planning easier since each
> check_query would count as 1 test.  (inspecting old pre-import pg_rewind repos
> on Github didn't given any other insights).  Does anymore remember the
> rationale for this?
>
> Since we moved to done_testing() with 549ec201d we no longer need be concerned
> with test counts, so we can replace this with normal is() tests, as per the
> attached, making the output in the errorpath consistent with other tests.
> Unless I'm missing something important here.


Looks OK. Now we require a sufficiently modern Test::More we could have
made it a subtest if necessary.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: making pg_regress less noisy by removing boilerplate

[Andrew Dunstan]

On 2/21/22 12:31, Andres Freund wrote:
>
> We still have a few issues with ports conflicts on windows. We should really
> consider just desupporting all windows versions without unix socket
> support. But until then it seems useful to be able to figure out random
> failures.
>

I'm pretty sure all my Windows machines with buildfarm animals are
sufficiently modern except the XP machine, which only builds release 10
nowadays.

What's involved in moving to require Unix socket support?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: bailing out in tap tests nearly always a bad idea

[Andrew Dunstan]

On 2/22/22 13:19, Andres Freund wrote:
> Hi,
>
> On 2022-02-22 09:28:37 -0800, Andres Freund wrote:
>> On 2022-02-14 09:46:20 -0800, Andres Freund wrote:
>>>> t/die-immediately.t  aieee at t/die-immediately.t line 1.
>>>> t/die-immediately.t  Dubious, test returned 255 (wstat 65280, 0xff00)
>>>> No subtests run
>>> Hm. Looks different when a test is including our test helpers.
>>>
>>> t/000_die_pg_test_utils.pl .. Dubious, test returned 25 (wstat 6400, 0x1900)
>>> No subtests run
>>> t/000_die_test_more.pl .. error at t/000_die_test_more.pl line 2.
>>> t/000_die_test_more.pl .. Dubious, test returned 255 (wstat 65280, 
>>> 0xff00)
>>> No subtests run
>>>
>>> So I think we broke something... I think it's the log file redirection stuff
>>> in INIT.
>> Any chance somebody with more perl knowledge could look into this? Clearly 
>> our
>> effort to copy all the output the original file descriptors isn't successful
>> here.
>>
>> $ perl -I /home/andres/src/postgresql/src/test/perl/ -e "use 
>> PostgreSQL::Test::Utils; die 'blorb';"
>> $ perl -I /home/andres/src/postgresql/src/test/perl/ -e "use Test::More; die 
>> 'blorb';"
>> blorb at -e line 1.
> So the problem is that die just outputs to stderr, but we've decided to have
> stderr connected to the output going to tap for some reason. I guess that
> prevents us from messing up the tap output, but it's still weird. Because of
> that redirection, die printing to stderr isn't visible to tap.
>
> Seems we can use black magic to "fix" that... Putting the following into
> INIT{} seems to do the trick:
>
>   # Because of the above redirection the tap output wouldn't contain
>   # information about tests failing due to die etc. Fix that by also
>   # printing the failure to the original stderr.
>   $SIG{__DIE__} = sub
>   {
>   # Don't redirect if it's a syntax error ($^S is undefined) or 
> in an
>   # eval block ($^S == 1).
>   if (defined $^S and $^S == 0)
>   {
>   print $orig_stderr "$_[0]\n";
>   #fail("died: $_[0]");
>   #done_testing();
>   }
>   };
>
>
> $ cat /tmp/die_pg_utils.pl
> use PostgreSQL::Test::Utils;
> use Test::More;
>
> ok(1, "foo");
> die 'blorb';
> done_testing();
>
> With the print we get something like:
>
> $ perl -I /home/andres/src/postgresql/src/test/perl/ /tmp/die_pg_utils.pl
> ok 1 - foo
> blorb at /tmp/die_pg_utils.pl line 5.
>
> # Tests were run but no plan was declared and done_testing() was not seen.
> # Looks like your test exited with 25 just after 1.
>
> With the fail() and done_testing() we get
>
> $ perl -I /home/andres/src/postgresql/src/test/perl/ /tmp/die_pg_utils.pl
> ok 1 - foo
> not ok 2 - died: blorb at /tmp/die_pg_utils.pl line 5.
> #
> #   Failed test 'died: blorb at /tmp/die_pg_utils.pl line 5.
> # '
> #   at /home/andres/src/postgresql/src/test/perl/PostgreSQL/Test/Utils.pm 
> line 235.
> 1..2
> # Looks like your test exited with 25 just after 2.
>
>
> The latter output doesn't confuse with stuff about plans and exit codes. But
> contains redundant messages (whatever) and it doesn't seem particularly clean
> to hijack a "test number".
>



I'll be surprised if we can't come up with something cleaner than that.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: bailing out in tap tests nearly always a bad idea

[Andrew Dunstan]

On 2/22/22 15:54, Andres Freund wrote:
> On 2022-02-22 15:10:30 -0500, Andrew Dunstan wrote:
>> I'll be surprised if we can't come up with something cleaner than that.
> Suggestions?


If we just have the sig handler actions as:

    diag("died: $_[0]");
    done_testing();   

we get:

    ok 1 - foo
    # died: blorb at tst/tst.pl line 5.
    1..1
    # Looks like your test exited with 25 just after 1.


Would that work?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: List of all* PostgreSQL EXTENSIONs in the world

[Andrew Dunstan]

On 2/23/22 03:52, Joel Jacobson wrote:
>
>> (e.g. I see three
>> instances of
>> blackhole_fdw listed, but not the original repo, which is not even on
>> GitHub) etc..
> Thanks, I've fixed blackhole_fdw manually:
>
> - https://bitbucket.org/adunstan/blackhole_fdw
> - https://github.com/chenquanzhao/blackhole_fdw
>


Yeah, I have several others on bitbucket that might be of interest (in
varying states of doneness):


A thin layer over the Redis API:

https://bitbucket.org/adunstan/redis_wrapper


Generate a CSV line from a row:

https://bitbucket.org/adunstan/row_to_csv


Closed forms of discrete builtin ranges:

https://bitbucket.org/adunstan/pg-closed-ranges


FDW to generate random data:

https://bitbucket.org/adunstan/rotfang-fdw


Comparison ops for JSON:

https://bitbucket.org/adunstan/jsoncmp



cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: making pg_regress less noisy by removing boilerplate

[Andrew Dunstan]

On 2/22/22 17:06, Andres Freund wrote:
>> What's involved in moving to require Unix socket support?
> It works today (the CI scripts use it on windows for example).
>
> But it's awkward because make_temp_sockdir() defaults to /tmp/ if TMPDIR isn't
> set. Which it is not by default on windows. There's PG_REGRESS_SOCK_DIR, which
> kind of works for the msvc build, because pg_regress tests aren't run
> concurrently (whereas tap tests can be run concurrently with
> PROVE_FLAGS-j).
>
> I think we just make make_temp_sockdir() a tad smarter. Perhaps by lifting the
> code in src/bin/psql/command.c:do_edit() to src/port/path.c or such?
>

+1 for doing that.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: bailing out in tap tests nearly always a bad idea

[Andrew Dunstan]

On 2/23/22 11:40, Andres Freund wrote:
> Hi,
>
> On 2022-02-23 08:43:38 -0500, Andrew Dunstan wrote:
>> On 2/22/22 15:54, Andres Freund wrote:
>>> On 2022-02-22 15:10:30 -0500, Andrew Dunstan wrote:
>>>> I'll be surprised if we can't come up with something cleaner than that.
>>> Suggestions?
>>
>> If we just have the sig handler actions as:
>>
>>     diag("died: $_[0]");
>>     done_testing();   
>>
>> we get:
>>
>>     ok 1 - foo
>>     # died: blorb at tst/tst.pl line 5.
>>     1..1
>>     # Looks like your test exited with 25 just after 1.
>>
>>
>> Would that work?
> Well, the if condition I had is needed, afaics. Otherwise we break eval() and
> / or risk crashing on syntax errors.  If you're just talking about diag() vs
> ok(0, ...), I'm good with that.



Yes, sorry, I meant this should be the contents of the condition block.
I agree completely that it's necessary.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: [PATCH] Enable SSL library detection via PQsslAttribute

[Andrew Dunstan]

On 2/23/22 13:38, Jacob Champion wrote:
> Hello,
>
> As part of the NSS work it came up [1] that clients don't have a good
> way to ask libpq what SSL library it was compiled with, unless they
> already have a connection pointer so that they can call
> PQsslAttribute(conn, "library"). This poses a chicken-and-egg problem:
> with the NSS proposal, the client may have to know which library is in
> use before it can construct a proper connection string. For example, I
> have a test suite that needs to set up an NSS database with
> certificates before telling libpq to connect using that database.
>
> The simplest proposal was to just allow PQsslAttribute() to take NULL
> as the connection parameter when querying the "library" attribute, and
> that's what I've done in this patch. In current versions of libpq, the
> "library" attribute will always be NULL if you pass NULL as the
> connection; a client that needs to know whether this new behavior is
> present can look at the LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro.
>
> If this looks good, I'm not sure how best to test it in the regression
> suite. I see that libpq has an installcheck recipe that compiles a test
> executable for URI parsing; should I add a simple test alongside that?


Create a TAP tests that calls a small client?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: convert libpq uri-regress tests to tap test

[Andrew Dunstan]

On 2/23/22 15:30, Andres Freund wrote:
> Hi,
>
> When verifying that the meson port actually runs all perl based tests I came
> across src/interfaces/libpq/test/regress.pl.  Instead of running tests yet
> another way, it seems better to convert it to a tap test.
>
> I hope others agree?


yes


>
> Where would we want that test to live? Right now we have the slightly odd
> convention that some tap tests live in src/test/{misc,modules,...}. But
> e.g. frontend binary ones are below src/bin/.
>
> For now I've left it in src/interfaces/libpq/test, with the test in
> t/001_uri.pl. But we should at least get rid of the test/...
>
> Perhaps we should just rename src/test/modules/libpq_pipeline to
> src/test/modules/libpq and move uri-regress in there as well?



WFM


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: convert libpq uri-regress tests to tap test

[Andrew Dunstan]

On 2/23/22 16:16, Andres Freund wrote:
> Hi,
>
> On 2022-02-23 15:57:08 -0500, Tom Lane wrote:
>> Andrew Dunstan  writes:
>>> On 2/23/22 15:30, Andres Freund wrote:
>>>> Perhaps we should just rename src/test/modules/libpq_pipeline to
>>>> src/test/modules/libpq and move uri-regress in there as well?
>>> WFM
>> +1
> Cool.
>
>
> One question on making that happen: Right now the makefiles building C stuff
> in src/test/modules all have the contrib-style stanza to build via pgxs.  But
> afaics pgxs doesn't support building multiple programs. Which
> src/test/modules/libpq would need to.


That's my understanding also.


>
> Aaics there's currently no way to have Mkvcbuild.pm build multiple programs in
> one dir via its makefile parsing stuff? Andrew, any suggestions for not
> needing to spell this out in both the makefile and Mkvcbuild.pm?


Well, it should be a SMOC. If you can solve the first problem I'm sure I
can come up with a solution for Mkvcbuild.pm. But until we see the shape
of the pgxs changes, planning for Mkcvbuild.pm changes seems remature.


>
>
> Separately: I don't really understand why we do the whole if USE_PGXS dance in
> src/test/modules?
>

ENOCLUE


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: convert libpq uri-regress tests to tap test

[Andrew Dunstan]

On 2/23/22 20:52, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 23.02.22 23:58, Tom Lane wrote:
>>> Peter Eisentraut  writes:
>>>> libpq TAP tests should be in src/interfaces/libpq/t/.
>>> That's failing to account for the fact that a libpq test can't
>>> really be a pure-perl TAP test; you need some C code to drive the
>>> library.
>> Such things could be put under src/interfaces/libpq/test, or some other 
>> subdirectory.  We already have src/interfaces/ecpg/test.
> OK, but then the TAP scripts are under src/interfaces/libpq/test/t,
> which isn't what you said.  I have no great objection to moving
> src/test/modules/libpq_pipeline/ to src/interfaces/libpq/test/,
> though, as long as the buildfarm will cope.
>
>   


It won't without some adjustment.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Proposal: Support custom authentication methods using hooks

[Andrew Dunstan]

On 2/24/22 04:16, Aleksander Alekseev wrote:
> Hi Samay,
>
>> I wanted to submit a patch to expose 2 new hooks (one for the authentication 
>> check and another one for error reporting) in auth.c. These will allow users 
>> to implement their own authentication methods for Postgres or add custom 
>> logic around authentication.
> I like the idea - PostgreSQL is all about extendability. Also, well
> done with TAP tests and an example extension. This being said, I
> didn't look at the code yet, but cfbot seems to be happy with it:
> http://cfbot.cputube.org/
>
>> One constraint in the current implementation is that we allow only one 
>> authentication provider to be loaded at a time. In the future, we can add 
>> more functionality to maintain an array of hooks and call the appropriate 
>> one based on the provider name in the pg_hba line.
> This sounds like a pretty severe and unnecessary limitation to me. Do
> you think it would be difficult to bypass it in the first
> implementation?



Yeah, I think we  would want a set of providers that could be looked up
at runtime.


I think this is likely to me material for release 16, so there's plenty
of time to get it right.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: set TESTDIR from perl rather than Makefile

[Andrew Dunstan]

On 2/24/22 20:17, Justin Pryzby wrote:
> On Mon, Feb 21, 2022 at 07:00:54AM -0500, Andrew Dunstan wrote:
>> On 2/19/22 18:53, Justin Pryzby wrote:
>>> On Sat, Feb 19, 2022 at 05:41:49PM -0600, Justin Pryzby wrote:
>>>> I rebased and fixed the check-guc script to work, made it work with vpath
>>>> builds, and cleaned it up some.
>>> I also meant to also attach it.
>> This is going to break a bunch of stuff as written.
>>
>> First, it's not doing the same thing. The current system sets TESTDIR to
>> be the parent of the directory that holds the test. e.g. for
>> src/bin/pg_ctl/t/001_start_stop.pl it's src/bin/pg_ctl in the build
>> tree, not the 't' subdirectory. This patch apparently sets it to the 't'
>> subdirectory. That will break anything that goes looking for log files
>> in the current location, like the buildfarm client, and possibly some CI
>> setups as well.
> Yes, thanks.
>
> I changed the patch to use ENV{CURDIR} || dirname(dirname($0)).  If I'm not
> wrong, that seems to be doing the right thing.
>
>> Also, unless I'm mistaken it appears to to the wrong thing for vpath
>> builds:
>>
>> my $test_dir = File::Spec->rel2abs(dirname($0));
>>
>> is completely wrong for vpaths, since that will place it in the source
>> tree, not the build tree.
>>
>> Last, and for no explained reason that I can see, the patch undoes
>> commit f4ce6c4d3a, but only for msvc builds. Even if that's justified it
>> appears to have no relevance to this patch.
> Andres' idea is that perl should set TESTDIR and PATH.  Here I commented out
> PATH, and had the improbable issue that nothing seemed to be breaking,
> including the pipeline test under msvc.  It'd be helpful to know what
> configuration that breaks so I can test that it's broken and then test that
> it's fixed when set from within perl...


I'm fairly sure what's broken here is the MSVC install procedure, which
is massively overeager. We should fix that rather than take it for granted.


>
> I got busy here, and may not be able to progress this for awhile.



You have fixed the vpath issue. But the changes in vcregress.pl look
wrong to me.


-    $ENV{TESTDIR} = "$dir";


If we set PG_SUBDIR in the Makefile shouldn't we set it here too? Yes it
probably doesn't matter as our MSVC build system doesn't support vpath
builds, but it's good to maintain as much equivalence between the two as
possible. (Supporting vpath builds for MSVC might be a nice
student/intern project)


 my $module = basename $dir;
-    # add the module build dir as the second element in the PATH
-    $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;


See above comment re msvc install. If we're not reverting f4ce6c4d3a in
the Makefile we shouldn't be reverting here either.


+    # $ENV{VCREGRESS_MODE} = $Config;


Why is this commented out line here?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Readd use of TAP subtests

[Andrew Dunstan]

On 2/25/22 08:39, Peter Eisentraut wrote:
> On 24.02.22 16:00, Andres Freund wrote:
>> I've incidentally played with subtests yesterdays, when porting
>> src/interfaces/libpq/test/regress.pl to a tap test. Unfortunately it
>> seems
>> that subtests aren't actually specified in the tap format, and that
>> different
>> libraries generate different output formats. The reason this matters
>> somewhat
>> is that meson's testrunner can parse tap and give nicer progress / error
>> reports. But since subtests aren't in the spec it can't currently parse
>> them...
>
> Ok that's good to know.  What exactly happens when it tries to parse
> them?  Does it not count them or does it fail somehow?  The way the
> output is structured
>
> t/001_basic.pl ..
> # Subtest: vacuumlo --help
>     ok 1 - exit code 0
>     ok 2 - goes to stdout
>     ok 3 - nothing to stderr
>     1..3
> ok 1 - vacuumlo --help
>
> it appears that it should be able to parse it nonetheless and should
> just count the non-indented lines.


AIUI TAP consumers are supposed to ignore lines they don't understand.
The Node TAP setup produces output like this, so perl is hardly alone
here. See <https://node-tap.org/docs/api/subtests/>


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Readd use of TAP subtests

[Andrew Dunstan]

On 2/25/22 11:41, Andres Freund wrote:
> Hi,
>
> On 2022-02-25 09:43:20 -0500, Andrew Dunstan wrote:
>> AIUI TAP consumers are supposed to ignore lines they don't understand.
> Are they?
>
> In http://testanything.org/tap-version-13-specification.html there's:
>
> "Lines written to standard output matching /^(not )?ok\b/ must be interpreted
> as test lines. [...]All other lines must not be considered test output."
>
> That says that all other lines aren't "test ouput". But what does that mean?
> It certainly doesn't mean they can just be ignored, because obviously
> ^(TAP version|#|1..|Bail out) isn't to be ignored.



I don't think we're following spec 13.


>
> And then there's:
>
>
> "
> Anything else
>
>   Any output that is not a version, a plan, a test line, a YAML block, a
>   diagnostic or a bail out is incorrect. How a harness handles the incorrect
>   line is undefined. Test::Harness silently ignores incorrect lines, but will
>   become more stringent in the future. TAP::Harness reports TAP syntax errors 
> at
>   the end of a test run.
> "
>
> If I were to to implement a tap parser this wouldn't make me ignore lines.
>
>
> Contrasting to that:
> http://testanything.org/tap-specification.html
>
> "
> Anything else
>
>   A TAP parser is required to not consider an unknown line as an error but may
>   optionally choose to capture said line and hand it to the test harness,
>   which may have custom behavior attached. This is to allow for forward
>   compatability. Test::Harness silently ignores incorrect lines, but will
>   become more stringent in the future. TAP::Harness reports TAP syntax errors
>   at the end of a test run.
> "
>
> I honestly don't know what to make of that. Parsers are supposed to ignore
> unknown lines, Test::Harness silently ignores, but also "TAP::Harness reports
> TAP syntax errors"?  This seems like a self contradictory mess.
>

I agree it's a mess. Both of these "specs" are incredibly loose. I guess
that happens when the spec comes after the implementation.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Mingw task for Cirrus CI

[Andrew Dunstan]

On 2/25/22 19:27, Andres Freund wrote:
> Hi,
>
> Andrew, CCIng you both because you might be interested in the CI bit, and
> because you might know the answer.
>
>
>
>> 2-  src/pl/plpython/Makefile looks under "C:/Windows/system32/" for
>> PYTHONDLL. gendef cannot open that file, give an error like " failed to
>> open()" when creating a .def file.
> On my win10 VM in which I installed python.org python I don't see a python dll
> in c:/windows/system32 either. Looks like none of our windows mingw animals
> build with python. So it might just be bitrot.



There certainly was a time when python from python.org used to install
its DLL in the system32 directory, so I imagine that's why it's there.
I'm very glad to see that's no longer the case.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

MSVC build system installs extra executables

[Andrew Dunstan]

I don't know if this has been discussed before, but I mentioned recently
(<https://www.postgresql.org/message-id/e4233934-98a6-6f76-46a0-992c0f4f1208%40dunslane.net>)
that I think the MSVC build system is too eager about installing
executables it builds. In particular, it installs these binaries for
which the analogs are not installed by the makefile system:


isolationtester.exe

libpq_pipeline.exe

pg_isolation_regress.exe

pg_regress_ecpg.exe

pg_regress.exe

zic.exe


Do we want to do anything about that? ISTM we should be installing
identical sets of binaries as far as possible. The installation of
libpq_pipeline.exe is apparently what has led Justin Pryzby to think
it's OK to undo the effect of commit f4ce6c4d3a on vcregress.pl.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Overflow of attmissingval is not handled gracefully

[Andrew Dunstan]

On 2/28/22 18:21, Tom Lane wrote:
> Consider this admittedly-rather-contrived example:
>
> regression=# create table foo(f1 int);
> CREATE TABLE
> regression=# alter table foo add column bar text default repeat('xyzzy', 
> 100);
> ERROR:  row is too big: size 57416, maximum size 8160
>
> Since the table contains no rows at all, this is a surprising
> failure.  The reason for it of course is that pg_attribute
> has no TOAST table, so it can't store indefinitely large
> attmissingval fields.
>
> I think the simplest answer, and likely the only feasible one for
> the back branches, is to disable the attmissingval optimization
> if the proposed value is "too large".  Not sure exactly where the
> threshold for that ought to be, but maybe BLCKSZ/8 could be a
> starting offer.
>
>   


WFM. After all, it's taken several years for this to surface. Is it
based on actual field experience?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: convert libpq uri-regress tests to tap test

[Andrew Dunstan]

On 2/24/22 07:19, Andrew Dunstan wrote:
> On 2/23/22 20:52, Tom Lane wrote:
>> Peter Eisentraut  writes:
>>> On 23.02.22 23:58, Tom Lane wrote:
>>>> Peter Eisentraut  writes:
>>>>> libpq TAP tests should be in src/interfaces/libpq/t/.
>>>> That's failing to account for the fact that a libpq test can't
>>>> really be a pure-perl TAP test; you need some C code to drive the
>>>> library.
>>> Such things could be put under src/interfaces/libpq/test, or some other 
>>> subdirectory.  We already have src/interfaces/ecpg/test.
>> OK, but then the TAP scripts are under src/interfaces/libpq/test/t,
>> which isn't what you said.  I have no great objection to moving
>> src/test/modules/libpq_pipeline/ to src/interfaces/libpq/test/,
>> though, as long as the buildfarm will cope.
>>
>>  
>
> It won't without some adjustment.



See
<https://github.com/PGBuildFarm/client-code/commit/ffc0fc029877632e9437af51bd99ace308daf0c8>
and
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2022-03-01%2010%3A47%3A22&stg=module-libpq-check>


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: functions

[Andrew Dunstan]

On 2/1/22 14:11,I wrote:
>
> 2. The new GUC "sql_json" is a bit of a worry. I understand what it's
> trying to do, but I'm trying to convince myself it's not going to be a
> fruitful source of error reports, especially if people switch it in the
> middle of a session. Maybe it should be an initdb option instead of a GUC?
>
>


So far my efforts have not borne fruit. Here's why:


andrew=# set sql_json = jsonb;
SET
andrew=# create table abc (x text, y json);
CREATE TABLE
andrew=# \d abc
   Table "public.abc"
 Column | Type | Collation | Nullable | Default
+--+---+--+-
 x  | text |   |  |
 y  | json |   |  |

andrew=# insert into abc values ('a','{"q":1}');
INSERT 0 1
andrew=# select json_each(y) from abc;
ERROR:  function json_each(json) does not exist
LINE 1: select json_each(y) from abc;
   ^
HINT:  No function matches the given name and argument types. You might
need to add explicit type casts.
andrew=# select jsonb_each(y) from abc;
 jsonb_each

 (q,1)
(1 row)


The description tells them the column is json, but the json_* functions
don't work on the column and you need to use the jsonb functions. That
seems to me a recipe for major confusion. It might be better if we set
it at initdb time so it couldn't be changed, but even so it could be
horribly confusing.

This is certainly severable from the rest of these patches. I'm not sure
how severable it is from the SQL/JSON Table patches.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: functions

[Andrew Dunstan]

On 3/1/22 16:41, Andrew Dunstan wrote:
> On 2/1/22 14:11,I wrote:
>> 2. The new GUC "sql_json" is a bit of a worry. I understand what it's
>> trying to do, but I'm trying to convince myself it's not going to be a
>> fruitful source of error reports, especially if people switch it in the
>> middle of a session. Maybe it should be an initdb option instead of a GUC?
>>
>>
>
> So far my efforts have not borne fruit. Here's why:
>
>
> andrew=# set sql_json = jsonb;
> SET
> andrew=# create table abc (x text, y json);
> CREATE TABLE
> andrew=# \d abc
>    Table "public.abc"
>  Column | Type | Collation | Nullable | Default
> +--+---+--+-
>  x  | text |   |  |
>  y  | json |   |  |
>
> andrew=# insert into abc values ('a','{"q":1}');
> INSERT 0 1
> andrew=# select json_each(y) from abc;
> ERROR:  function json_each(json) does not exist
> LINE 1: select json_each(y) from abc;
>    ^
> HINT:  No function matches the given name and argument types. You might
> need to add explicit type casts.
> andrew=# select jsonb_each(y) from abc;
>  jsonb_each
> 
>  (q,1)
> (1 row)
>
>
> The description tells them the column is json, but the json_* functions
> don't work on the column and you need to use the jsonb functions. That
> seems to me a recipe for major confusion. It might be better if we set
> it at initdb time so it couldn't be changed, but even so it could be
> horribly confusing.
>
> This is certainly severable from the rest of these patches. I'm not sure
> how severable it is from the SQL/JSON Table patches.
>
>

I have confirmed that this is not required at all for the JSON_TABLE
patch set.


I'll submit new patch sets omitting it shortly. The GUC patch can be
considered separately, probably as release 16 material, but I think as
is it's at best quite incomplete.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Mingw task for Cirrus CI

[Andrew Dunstan]

On 3/3/22 05:16, Melih Mutlu wrote:
> Hi Andres, 
>  
>
> This presumably is due to using mingw's python rather than
> python.org <http://python.org>'s
> python. Seems like a reasonable thing to support for the mingw build.
>
> Melih, you could try to build against the python.org
> <http://python.org> python (i installed in
> the CI container).
>
>
> I tried to use the installed python from python.org
> <http://python.org> in the container. 
> The problem with this is that "LIBDIR" and "LDLIBRARY" configs of
> python for windows from python.org <http://python.org> are empty.
> Therefore python_libdir or other related variables in configure file
> are not set correctly.



Yeah, here's what it has:


# python -c "import sysconfig; import pprint; pp =
pprint.PrettyPrinter(); pp.pprint(sysconfig.get_config_vars())"
{'BINDIR': 'C:\\prog\\python310',
 'BINLIBDEST': 'C:\\prog\\python310\\Lib',
 'EXE': '.exe',
 'EXT_SUFFIX': '.cp310-win_amd64.pyd',
 'INCLUDEPY': 'C:\\prog\\python310\\Include',
 'LIBDEST': 'C:\\prog\\python310\\Lib',
 'SO': '.cp310-win_amd64.pyd',
 'TZPATH': '',
 'VERSION': '310',
 'abiflags': '',
 'base': 'C:\\prog\\python310',
 'exec_prefix': 'C:\\prog\\python310',
 'installed_base': 'C:\\prog\\python310',
 'installed_platbase': 'C:\\prog\\python310',
 'platbase': 'C:\\prog\\python310',
 'platlibdir': 'lib',
 'prefix': 'C:\\prog\\python310',
 'projectbase': 'C:\\prog\\python310',
 'py_version': '3.10.2',
 'py_version_nodot': '310',
 'py_version_nodot_plat': '310',
 'py_version_short': '3.10',
 'srcdir': 'C:\\prog\\python310',
 'userbase': 'C:\\Users\\Administrator\\AppData\\Roaming\\Python'}


The DLL lives in the BINDIR, so maybe I guess we should search there if
we can't get the other things.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

[Andrew Dunstan]

On 3/3/22 00:03, Michael Paquier wrote:
>>> +if (   (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
>>> +   || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
>> Odd indentation. Spaces between parens?
> Well, perltidy tells me that this is right.
>
>

Yeah, I haven't found a way to make it stop doing that :-(


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: wal_compression=zstd

[Andrew Dunstan]

On 2/22/22 18:19, Justin Pryzby wrote:
> As writen, this patch uses zstd level=1 (whereas the ZSTD's default compress
> level is 6).


I think this choice needs to be supported by some benchmarks.


>
> 0001 adds support for zstd
> 0002 makes more efficient our use of bits in the WAL header
> 0003 makes it the default compression, to exercise on CI (windows will fail).
> 0004 adds support for zstd-level
> 0005-6 are needed to allow recovery tests to pass when wal compression is 
> enabled;
> 0007 (not included) also adds support for zlib.  I'm of the impression nobody
>  cares about this, otherwise it would've been included 5-10 years ago.
>
> Only 0001 should be reviewed for pg15 - the others are optional/future work.



I don't see why patch 5 shouldn't be applied forthwith.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 3/4/22 13:13, Erikjan Rijkers wrote:
> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>
>
>> This set of patches deals with items 1..7 above, but not yet the ERROR
>> ON ERROR issue. It also makes some message cleanups, but there is more
>> to come in that area.
>>
>> It is based on the latest SQL/JSON Functions patch set, which does not
>> include the sql_json GUC patch.
>>
> > [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
> > [0002-JSON_TABLE-v56.patch]
> > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
> > [0004-JSON_TABLE-PLAN-clause-v56.patch]
>
> Hi,
>
> I quickly tried the tests I already had and there are two statements
> that stopped working:
>
> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
> RETURNING jsonb);
> ERROR:  syntax error at or near "RETURNING"
> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
> ...
>
> testdb=# select JSON_SCALAR(123.45 returning jsonb);
> ERROR:  syntax error at or near "returning"
> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>
>   (the '^' pointer in both cases underneath 'RETURNING'
>
>
>


Yes, you're right, that was implemented as part of the GUC patch. I'll
try to split that out and send new patchsets with the RETURNING clause
but without the GUC (see upthread for reasons)


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Regression tests failures on Windows Server 2019 - on master at commit # d816f366b

[Andrew Dunstan]

On 3/4/22 17:04, Andres Freund wrote:
> Hi,
>
> On 2022-03-04 16:51:32 -0500, Tom Lane wrote:
>> Andres Freund  writes:
>>> That fixed the immediate problem, but dblink, postgres_fdw tests failed:
>>> +ERROR:  could not establish connection
>>> +DETAIL:  connection to server at "localhost" (::1), port 5432 failed: 
>>> FATAL:
>>>  role "SYSTEM" does not exist
>> [ scratches head... ]  Where is libpq getting that username from?
>> Why is it different from whatever we'd determined during initdb?
>> (Maybe a case-folding issue??)
> When running as a service (via pg_ctl register) the default username to run
> under appears to be SYSTEM. Which then differs from the user that vcregress.pl
> runs under. Trying to make it use the current user now - I don't know what
> permissions services are needed to run a service as a user or such.


SeServiceLogonRight is what the user needs to run the service.


To manage it (e.g. stop or start it) they need some extra permissions,
see for example <http://get-carbon.org/Grant-ServicePermission.html>


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: [PoC] Let libpq reject unexpected authentication requests

[Andrew Dunstan]

On 3/4/22 20:19, Tom Lane wrote:
> Jacob Champion  writes:
>> $subject keeps coming up in threads. I think my first introduction to
>> it was after the TLS injection CVE, and then it came up again in the
>> pluggable auth thread. It's hard for me to generalize based on "sound
>> bites", but among the proposals I've seen are
>> 1. reject plaintext passwords
>> 2. reject a configurable list of unacceptable methods
>> 3. allow client and server to negotiate a method
>> All of them seem to have merit.
> Agreed.
>
>> Here is my take on option 2, then: you get to choose exactly one method
>> that the client will accept. If you want to use client certificates,
>> use require_auth=cert. If you want to force SCRAM, use
>> require_auth=scram-sha-256. If the server asks for something different,
>> libpq will fail. If the server tries to get away without asking you for
>> authentication, libpq will fail. There is no negotiation.
> Seems reasonable, but I bet that for very little more code you could
> accept a comma-separated list of allowed methods; libpq already allows
> comma-separated lists for some other connection options.  That seems
> like it'd be a useful increment of flexibility.
>
>   


Just about necessary I guess, since you can specify that a client cert
is required in addition to some other auth method, so for such cases you
might want something like "required_auth=cert,scram-sha-256"? Or do we
need a way of specifying the combination?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: ltree_gist indexes broken after pg_upgrade from 12 to 13

[Andrew Dunstan]

On 3/4/22 15:28, Tom Lane wrote:
> Tomas Vondra  writes:
>> On 3/4/22 20:29, Nikita Glukhov wrote:
>>> So, we probably have corrupted indexes that were updated since such 
>>> "incomplete" upgrade of ltree.
>> IIRC pg_upgrade is not expected to upgrade extensions - it keeps the
>> installed version of the extension, and that's intentional.
> Yeah, exactly.  But this opens up an additional consideration we
> have to account for: whatever we do needs to work with either 1.1
> or 1.2 SQL-level versions of the extension.
>
>   


This is an area not currently touched by the buildfarm's cross version
upgrade testing, which basically compares a pre-upgrade and post-upgrade
dump of the databases. The upgraded cluster does contain
contrib_regression_ltree.

I'm open to suggestions on how we might improve the buildfarm's testing
of upgraded indexes generally.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Time to drop plpython2?

[Andrew Dunstan]

On 3/8/22 15:02, Andres Freund wrote:
> Hi,
>
> On 2022-03-08 10:42:31 -0800, Andres Freund wrote:
>>> crake also failed. Looks like plpy_plpymodule.h needs to include 
>>> plpython.h. A
>>> pre-existing issue that just didn't happen to cause problems...
>> Fixed that.
> Hm. Now crake failed in XversionUpgrade-REL9_2_STABLE-HEAD:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2022-03-08%2018%3A47%3A22
>
> except that the log doesn't actually indicate any problem? Andrew, any hint?
>


That was a snafu on my part, as I was adding extension upgrade / amcheck
testing. It's fixed now, so please disregard this one.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: ltree_gist indexes broken after pg_upgrade from 12 to 13

[Andrew Dunstan]

On 3/6/22 17:33, Andres Freund wrote:
> Hi,
>
> On 2022-03-06 07:46:04 -0500, Andrew Dunstan wrote:
>> This is an area not currently touched by the buildfarm's cross version
>> upgrade testing, which basically compares a pre-upgrade and post-upgrade
>> dump of the databases. The upgraded cluster does contain
>> contrib_regression_ltree.
>>
>> I'm open to suggestions on how we might improve the buildfarm's testing
>> of upgraded indexes generally.
> One thing that's likely worth doing as part of the cross version upgrade test,
> even if it wouldn't even help in this case, is to run amcheck post
> upgrade. Just dumping data isn't going to touch indices at all.
>
> A sequence of
>   pg_upgrade; amcheck; upgrade all extensions; amcheck;
> would make sense.
>


See
<https://github.com/PGBuildFarm/client-code/commit/191df23bd25eb5546b0989d71ae92747151f9f39>



This will be in the next release


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: ltree_gist indexes broken after pg_upgrade from 12 to 13

[Andrew Dunstan]

On 3/10/22 10:53, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 3/6/22 17:33, Andres Freund wrote:
>>> A sequence of
>>> pg_upgrade; amcheck; upgrade all extensions; amcheck;
>>> would make sense.
>> See
>> <https://github.com/PGBuildFarm/client-code/commit/191df23bd25eb5546b0989d71ae92747151f9f39>
> Does this detect the problem at hand?
>
>   


AIUI, amcheck doesn't check gist indexes, hence Andres' "even if it
wouldn't even help in this case". So no.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan  wrote:
>>
>> rebased with some review comments attended to.
> I am in process of reviewing these patches, initially, have started
> with 0002-JSON_TABLE-v55.patch.
> Tested many different scenarios with various JSON messages and these
> all are working as expected. Just one question on the below output.
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON EMPTY)) jt;
>  a
> ---
>
> (1 row)
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON ERROR)) jt;
>  a
> ---
>
> (1 row)
>
> is not "ERROR ON ERROR" is expected to give error?


I think I understand what's going on here. In the first example 'ERROR
ON EMPTY' causes an error condition, but as the default action for an
error condition is to return null that's what happens. To get an error
raised you would need to say 'ERROR ON EMPTY ERROR ON ERROR'. I don't
know if that's according to spec. It seems kinda screwy, arguably a POLA
violation, although that would hardly be a first for the SQL Standards
body.  But I'm speculating here, I'm not a standards lawyer.

In the second case it looks like there isn't really an error. There
would be if you used 'strict' in the path expression.


This whole area needs more documentation.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Change the csv log to 'key:value' to facilitate the user to understanding and processing of logs

[Andrew Dunstan]

On 3/15/22 09:30, hubert depesz lubaczewski wrote:
> On Tue, Mar 15, 2022 at 09:31:19AM +0800, lupeng wrote:
>> Dear Hackers
>> When I audit the Postgresql database recently, I found that after
>> configuring the log type as csv, the output log content is as follows:
>> "database ""lp_db1"" does not exist","DROP DATABASE
>> lp_db1;",,"dropdb, dbcommands.c:841","","client backend",,0 It is very
>> inconvenient to understand the real meaning of each field. And in the
>> log content，" is escaped as "", which is not friendly to regular
>> expression matching. Therefore, I want to modify the csv log function,
>> change its format to key:value, assign the content of the non-existing
>> field to NULL, and at the same time, " will be escaped as  \" in the
>> log content. After the modification, the above log format is as
>> follows: Log_time:"2022-03-15 09:17:55.289
>> CST",User_name:"postgres",Database_name:"lp_db",Process_id:"17995",Remote_host:"192.168.88.130",Remote_port:"38402",Line_number:
>> "622fe941.464b",PS_display:"DROP
>> DATABASE",Session_start_timestamp:"2022-03-15 09:17:53
>> CST",Virtual_transaction_id:"3/2",Transaction_id:"NULL",Error_severity:"ERROR",SQL_state_code
>> :"3D000",Errmessage:"database \"lp_db1\" does not
>> exist",Errdetail:"NULL",Errhint:"NULL",Internal_query:"NULL",Internal_pos:"0",Errcontext:"NULL",User_query
>> :"DROP DATABASE lp_db1;",Cursorpos:"NULL",File_location:"dropdb,
>> dbcommands.c:841",Application_name:"NULL",Backend_type:"client
>> backend",Leader_PID:"0",Query_id:"0"
> CSV format is well documented
> (https://www.postgresql.org/docs/current/runtime-config-logging.html#RUNTIME-CONFIG-LOGGING-CSVLOG).
>
> If you want named fields you can wait for pg15 and its jsonlog
> (https://www.depesz.com/2022/01/17/waiting-for-postgresql-15-introduce-log_destinationjsonlog/).
>
> I, for one, wouldn't want to have to deal with field names repeated in
> every single record.
>

Indeed. And even if this were a good idea, which it's not, it would be
15 years too late.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Andrew Dunstan]

On 3/15/22 16:59, Mark Dilger wrote:
>> On Mar 6, 2022, at 3:27 PM, Tom Lane  wrote:
>>
>> Mark Dilger  writes:
>>> The existing patch allows grants on unknown gucs, because it can't know 
>>> what guc an upgrade script will introduce, and the grant statement may need 
>>> to execute before the guc exists.
>> Yeah, that's the problematic case.  It might mostly work to assume that
>> an unknown GUC has an empty default ACL.  This could fail to retain the
>> default PUBLIC SET permission if it later turns out the GUC is USERSET
> On further reflection, I concluded this isn't needed.  No current extension, 
> whether in-core or third party, expects to be able to create a new GUC and 
> then grant or revoke permissions on it.  They can already specify the guc 
> context (PGC_USERS, etc).  Introducing a feature that depends on the dubious 
> assumption that unrecognized GUCs will turn out to be USERSET doesn't seem 
> warranted.

Agreed.


>
> The patch attributes all grants of setting privileges to the bootstrap 
> superuser.  Only superusers can grant or revoke privileges on settings, and 
> all settings are implicitly owned by the bootstrap superuser because there is 
> no explicit owner associated with settings.  Consequently, 
> select_best_grantor(some_superuser, ..., BOOTSTRAP_SUPERUSERID, ...) always 
> chooses the bootstrap superuser.  I don't see a problem with this, but 
> wouldn't mind a second opinion.  Some people might find it surprising when 
> viewing the pg_setting_acl.setacl field.


I think it's OK as long as we document it. An alternative might be to
invent a pseudo-superuser called, say, 'postgres_system', but that seems
like overkill to solve what is in effect a cosmetic problem.

Generally I think this is now in fairly good shape, I've played with it
and it seems to do what I expect in every case, and the things I found
surprising are gone.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Andrew Dunstan]

On 3/16/22 14:47, Tom Lane wrote:
> Andrew Dunstan  writes:
>> Generally I think this is now in fairly good shape, I've played with it
>> and it seems to do what I expect in every case, and the things I found
>> surprising are gone.
> Stepping back a bit ... do we really want to institutionalize the
> term "setting" for GUC variables?  I realize that the view pg_settings
> exists, but the documentation generally prefers the term "configuration
> parameters".  Where config.sgml uses "setting" as a noun, it's usually
> talking about a specific concrete value for a parameter, and you can
> argue that the view's name comports with that meaning.  But you can't
> GRANT a parameter's current value.
>
> I don't have a better name to offer offhand --- I surely am not proposing
> that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x",
> because that's way too wordy.  But now is the time to bikeshed if we're
> gonna bikeshed, or else admit that we're not interested in precise
> vocabulary.
>
> I'm also fairly allergic to the way that this patch has decided to assign
> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
> is no existing precedent for that, and I think it's going to break
> client-side code that we don't need to break.  It's not coincidental that
> this forces weird changes in rules about whitespace in the has_privilege
> functions, for example; and if you think that isn't going to cause
> problems I think you are wrong.  Perhaps we could just use "SET" and
> "ALTER", or "SET" and "SYSTEM"?


That's going to look weird, ISTM. This is less clear about what it's
granting.

 GRANT ALTER ON SOMETHING shared_buffers TO myuser;

If you don't like that maybe ALTER_SYSTEM and SET_VALUE would work,
although mostly we have avoided things like that.

How about MODIFY instead of SET VALUE and CONFIGURE instead of ALTER SYSTEM?

Personally I don't have problem with the use of SETTING. I think the
meaning is pretty plain in context and unlikely to produce any confusion.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Andrew Dunstan]

On 3/16/22 16:53, Tom Lane wrote:

>> Personally I don't have problem with the use of SETTING. I think the
>> meaning is pretty plain in context and unlikely to produce any confusion.
> I'm just unhappy about the disconnect with the documentation.  I wonder
> if we could get away with s/configuration parameter/setting/g in the docs.
>
>   


I don't think we need to fix that here though. If you can live with
SETTING for now I will undertake to fix the docs post-feature-freeze if
necessary.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Andrew Dunstan]

On 3/17/22 10:47, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 16.03.22 19:47, Tom Lane wrote:
>>> ...  Perhaps we could just use "SET" and
>>> "ALTER", or "SET" and "SYSTEM"?
>> I think Oracle and MS SQL Server have many multi-word privilege names. 
>> So users are quite used to that.  And if we want to add more complex 
>> privileges, we might run out of sensible single words eventually.  So I 
>> would not exclude this approach.
> Well, I still say that "SET" is sufficient for the one privilege name
> (unless we really can't make Bison handle that, which I doubt).  But
> I'm willing to yield on using "ALTER SYSTEM" for the other.
>
> If we go with s/SETTING/PARAMETER/ as per your other message, then
> that would be adequately consistent with the docs I think.  So it'd
> be
>
> GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
>
> and the new catalog would be pg_parameter_acl, and so on.
>
>   



The upside of this is that it avoids the inelegant


    GRANT SET ON SETTING ...


But I was just looking again at the grammar, and the only reason we need
this keyword at all AFAICS is to disambiguate ALL [PRIVILEGES] cases. 
If we abandoned that for this form of GRANT/REVOKE I think we could
probably get away with


    GRANT { SET | ALTER SYSTEM } ON setting_name ...


I haven't tried it, so I could be all wrong.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/18/22 11:15, Mark Dilger wrote:
>
>> On Mar 18, 2022, at 7:16 AM, Joshua Brindle  
>> wrote:
>>
>> This is great, thank you for doing this. I didn't even realize the OAT
>> hooks had no regression tests.
>>
>> It looks good to me, I reviewed both and tested the module. I wonder
>> if the slight abuse of subid is warranted with brand new hooks going
>> in but not enough to object, I just hope this doesn't rise to the too
>> large to merge this late level.


The core code is extracted from a current CF patch, so I think in
principle it's OK.


I haven't looked at it in detail, but regarding the test code I'm not
sure why there's a .control file, since this isn't a loadable extension,
not why there's a test_oat_hooks.h file.


> The majority of the patch is regression testing code, stuff which doesn't get 
> installed.  It's even marked as NO_INSTALLCHECK, so it won't get installed 
> even as part of "make installcheck".  That seems safe enough to me.
>
> Not including tests of OAT seems worse, as it leaves us open to breaking the 
> behavior without realizing we've done so.  A refactoring of the core code 
> might cause hooks to be called in a different order, something which isn't 
> necessarily wrong, but should not be done unknowingly.
>

Yes, and in any case we've added test code after feature freeze in the past.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Out-of-tree certificate interferes ssltest

[Andrew Dunstan]

On 3/17/22 21:02, Michael Paquier wrote:
> On Thu, Mar 17, 2022 at 02:28:49PM +0100, Daniel Gustafsson wrote:
>> One small concern though. This hunk:
>>
>> +my $default_ssl_connstr = "sslkey=invalid sslcert=invalid 
>> sslrootcert=invalid sslcrl=invalid sslcrldir=invalid";
>> +
>>  $common_connstr =
>> -  "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR 
>> host=common-name.pg-ssltest.test";
>> +  "$default_ssl_connstr user=ssltestuser dbname=trustdb 
>> hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
>>  
>> ..together with the following changes along the lines of:
>>
>> -"$common_connstr sslrootcert=invalid sslmode=require",
>> +"$common_connstr sslmode=require",
>>
>> ..is making it fairly hard to read the test and visualize what the connection
>> string is and how the test should behave.  I don't have a better idea off the
>> top of my head right now, but I think this is an area to revisit and improve
>> on.
> I agree that this makes this set of three tests harder to follow, as
> we expect a root cert to *not* be set locally.  Keeping the behavior
> documented in each individual string would be better, even if that
> duplicates more the keys in those final strings.
>
> Another thing that Horiguchi-san has pointed out upthread (?) is 003,
> where it is also possible to trigger failures once the environment is
> hijacked.  The attached allows the full test suite to pass without
> issues on my side.



LGTM


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: speed up a logical replica setup

[Andrew Dunstan]

On 3/15/22 09:51, Peter Eisentraut wrote:
> On 21.02.22 13:09, Euler Taveira wrote:
>> A new tool called pg_subscriber does this conversion and is tightly
>> integrated
>> with Postgres.
>
> Are we comfortable with the name pg_subscriber?  It seems too general.
> Are we planning other subscriber-related operations in the future?  If
> so, we should at least make this one use a --create option or
> something like that.


Not really sold on the name (and I didn't much like the name
pglogical_create_subscriber either, although it's a cool facility and
I'm happy to see us adopting something like it).

ISTM we should have a name that conveys that we are *converting* a
replica or equivalent to a subscriber.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: A test for replay of regression tests

[Andrew Dunstan]

On 3/20/22 05:36, Thomas Munro wrote:
> On Sun, Mar 20, 2022 at 5:20 PM Thomas Munro  wrote:
>> Another failure under 027_stream_regress.pl:
>>
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2022-03-16%2005%3A58%3A05
>>
>>  vacuum   ... FAILED 3463 ms
>>
>> I'll try to come up with the perl needed to see the regression.diffs
>> next time...
> Here's my proposed change to achieve that.


I think that's OK.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Problem with moderation of messages with patched attached.

[Andrew Dunstan]

On 3/19/22 14:48, Andres Freund wrote:
> Hi,
>
> On 2022-03-03 13:37:35 +, Dave Page wrote:
>> On Thu, 3 Mar 2022 at 13:28, Pavel Borisov  wrote:
>>
>>> The mail system doesn't have the capability to apply different moderation
>>>> rules for people in that way I'm afraid.
>>>>
>>> Maybe then 2MB for everyone? Otherwise it's not so convenient. Lead to
>>> answers before the questions in the thread [1], seems weird.
>>>
>> Then someone will complain if their patch is 2.1MB! How often are messages
>> legitimately over 1MB anyway, even with a patch? I don't usually moderate
>> -hackers, so I don't know if this is a common thing or not.
> I don't think it's actually that rare. But most contributors writing that
> large patchsets know about the limit and work around it - I gzip patches when
> I see the email getting too large. But it's more annoying to work with for
> reviewers.
>
> It's somewhat annoying. If you e.g. append a few graphs of performance changes
> and a patch it's pretty easy to get into the range where compressing won't
> help anymore.
>
> And sure, any limit may be hit by somebody. But 1MB across the whole email
> seems pretty low these days.
>

Of course we could get complaints no matter what level we set the limit
at. I think raising it to 2Mb would be a reasonable experiment. If no
observable evil ensues then leave it that way. If it does then roll it
back. I agree that plain uncompressed patches are easier to deal with in
general.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/17/22 23:21, Mark Dilger wrote:
> Hackers,
>
> Over in [1], Joshua proposed a new set of Object Access Type hooks based on 
> strings rather than Oids.
>
> His patch was written to be applied atop my patch for granting privileges on 
> gucs.
>
> On review of his patch, I became uncomfortable with the complete lack of 
> regression test coverage.  To be fair, he did paste a bit of testing logic to 
> the thread, but it appears to be based on pgaudit, and it is unclear how to 
> include such a test in the core project, where pgaudit is not assumed to be 
> installed.
>
> First, I refactored his patch to work against HEAD and not depend on my GUCs 
> patch.  Find that as v1-0001.  The refactoring exposed a bit of a problem.  
> To call the new hook for SET and ALTER SYSTEM commands, I need to pass in the 
> Oid of a catalog table.  But since my GUC patch isn't applied yet, there 
> isn't any such table (pg_setting_acl or whatnot) to pass.  So I'm passing 
> InvalidOid, but I don't know if that is right.  In any event, if we want a 
> new API like this, we should think a bit harder about whether it can be used 
> to check operations where no table Oid is applicable.


My first inclination is to say it's probably ok. The immediately obvious
alternative would be to create yet another set of functions that don't
have classId parameters. That doesn't seem attractive.

Modulo that issue I think patch 1 is basically ok, but we should fix the
comments in objectaccess.c.  Rather than "It is [the] entrypoint ..." we
should have something like "Oid variant entrypoint ..." and "Name
variant entrypoint ...", and also fix the function names in the comments.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/21/22 15:57, Mark Dilger wrote:
>> On Mar 18, 2022, at 3:04 PM, Andrew Dunstan  wrote:
>>
>> I haven't looked at it in detail, but regarding the test code I'm not
>> sure why there's a .control file, since this isn't a loadable extension,
>> not why there's a test_oat_hooks.h file.
> The .control file exists because the test defines a loadable module which 
> defines the hooks. 



To the best of my knowledge .control files are only used by extensions,
not by other modules. They are only referenced in
src/backend/commands/extension.c in the backend code. For example,
auto_explain which is a loadable module but not en extension does not
have one, and I bet if you remove it you'll find this will work just fine.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 01:03, Thomas Munro wrote:
> On Fri, Mar 18, 2022 at 4:22 PM Mark Dilger
>  wrote:
>> (FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl 
>> when testing v1-0001.  I'm not sure yet what that is about.)
> Doesn't look like 0001 has anything to do with that...   Are you on a
> Mac?  Did it look like this recent failure from CI?


Probably not connected. It's working fine for me on Ubuntu/


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 3/22/22 09:28, Oleg Bartunov wrote:
>
>
> On Tue, Mar 22, 2022 at 12:53 PM Matthias Kurz 
> wrote:
>
> Hi everyone!
>
> I am watching this thread since quite a while and I am waiting
> eagerly a long time already that this feature finally lands in
> PostgreSQL.
> Given that in around 2 weeks PostgreSQL 15 will go into feature
> freeze (in the last years that usually happened around the 8th of
> April AFAIK), is there any chance this will be committed? As far
> as I understand the patches are almost ready.
>
>
> We are waiting too :)


I'm planning on pushing the functions patch set this week and json-table
next week.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/21/22 19:08, Mark Dilger wrote:
>
>> On Mar 21, 2022, at 1:30 PM, Andrew Dunstan  wrote:
>>
>> To the best of my knowledge .control files are only used by extensions,
>> not by other modules. They are only referenced in
>> src/backend/commands/extension.c in the backend code. For example,
>> auto_explain which is a loadable module but not en extension does not
>> have one, and I bet if you remove it you'll find this will work just fine.
> Fixed, also with adjustments to Joshua's function comments.
>

Pushed with slight adjustments - the LOAD was unnecessary as was the
setting of client_min_messages - the latter would have made buildfarm
animals unhappy.


Now you need to re-submit your GUCs patch I think.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 3/22/22 10:53, Alvaro Herrera wrote:
> On 2022-Mar-22, Andrew Dunstan wrote:
>
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> I think it'd be a good idea to push the patches one by one and let a day
> between each.  That would make it easier to chase buildfarm issues
> individually, and make sure they are fixed before the next step.
> Each patch in each series is already big enough.



OK, can do it that way. First one will be later today then.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 3/22/22 10:55, Daniel Gustafsson wrote:
>> On 22 Mar 2022, at 16:31, Andrew Dunstan  wrote:
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> My comments from 30827b3c-edf6-4d41-bbf1-298181874...@yesql.se are yet to be
> addressed (or at all responded to) in this patchset.  I'll paste the ones 
> which
> still apply to make it easier:


Thanks for reminding me. I will make sure these are attended to.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 11:26, Mark Dilger wrote:
>
>> On Mar 22, 2022, at 8:14 AM, Julien Rouhaud  wrote:
>>
>> Hi,
>>
>> On Tue, Mar 22, 2022 at 10:41:05AM -0400, Andrew Dunstan wrote:
>>> Pushed with slight adjustments - the LOAD was unnecessary as was the
>>> setting of client_min_messages - the latter would have made buildfarm
>>> animals unhappy.
>> For the record this just failed on my buildfarm animal:
>> https://brekka.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&dt=2022-03-22%2014%3A40%3A10&stg=misc-check.
> culicidae is complaining:
>
> ==~_~===-=-===~_~== 
> pgsql.build/src/test/modules/test_oat_hooks/log/postmaster.log 
> ==~_~===-=-===~_~==
> 2022-03-22 14:53:27.175 UTC [2166986][postmaster][:0] LOG:  starting 
> PostgreSQL 15devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 11.2.0-18) 
> 11.2.0, 64-bit
> 2022-03-22 14:53:27.175 UTC [2166986][postmaster][:0] LOG:  listening on Unix 
> socket "/tmp/pg_regress-RiE7x8/.s.PGSQL.6280"
> 2022-03-22 14:53:27.198 UTC [2167008][not initialized][:0] FATAL:  
> test_oat_hooks must be loaded via shared_preload_libraries
> 2022-03-22 14:53:27.202 UTC [2167006][not initialized][:0] FATAL:  
> test_oat_hooks must be loaded via shared_preload_libraries
> 2022-03-22 14:53:27.203 UTC [2167009][not initialized][:0] FATAL:  
> test_oat_hooks must be loaded via shared_preload_libraries
> 2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG:  checkpointer 
> process (PID 2167006) exited with exit code 1
> 2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG:  terminating any 
> other active server processes
> 2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG:  shutting down 
> because restart_after_crash is off
> 2022-03-22 14:53:27.206 UTC [2166986][postmaster][:0] LOG:  database system 
> is shut down
> ==~_~===-=-===~_~== 
> pgsql.build/src/test/modules/test_rls_hooks/log/initdb.log ==~_~===-=-===~_~==
>
>


That seems quite weird. I'm not sure how it's getting loaded at all if
not via shared_preload_libraries


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 12:02, Tom Lane wrote:
> Andrew Dunstan  writes:
>> That seems quite weird. I'm not sure how it's getting loaded at all if
>> not via shared_preload_libraries
> Some other animals are showing this:
>
> diff -U3 
> /home/postgres/pgsql/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
>  
> /home/postgres/pgsql/src/test/modules/test_oat_hooks/results/test_oat_hooks.out
> --- 
> /home/postgres/pgsql/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
>   2022-03-22 11:57:40.224991011 -0400
> +++ 
> /home/postgres/pgsql/src/test/modules/test_oat_hooks/results/test_oat_hooks.out
>2022-03-22 11:59:59.998983366 -0400
> @@ -48,6 +48,8 @@
>  SELECT * FROM regress_test_table;
>  NOTICE:  in executor check perms: superuser attempting execute
>  NOTICE:  in executor check perms: superuser finished execute
> +NOTICE:  in executor check perms: superuser attempting execute
> +NOTICE:  in executor check perms: superuser finished execute
>   t 
>  ---
>  (0 rows)
> @@ -95,6 +97,8 @@
>^
>  NOTICE:  in executor check perms: non-superuser attempting execute
>  NOTICE:  in executor check perms: non-superuser finished execute
> +NOTICE:  in executor check perms: non-superuser attempting execute
> +NOTICE:  in executor check perms: non-superuser finished execute
>   t 
>  ---
>  (0 rows)
> @@ -168,6 +172,8 @@
>^
>  NOTICE:  in executor check perms: superuser attempting execute
>  NOTICE:  in executor check perms: superuser finished execute
> +NOTICE:  in executor check perms: superuser attempting execute
> +NOTICE:  in executor check perms: superuser finished execute
>   t 
>  ---
>  (0 rows)
>
>
> I can duplicate that by adding "force_parallel_mode = regress"
> to test_oat_hooks.conf, so a fair bet is that the duplication
> comes from executing the same hook in both leader and worker.
>
>   



OK, thanks. My test didn't include that one setting :-(


If I can't com up with a very quick fix I'll revert it.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 12:58, Tom Lane wrote:
> Mark Dilger  writes:
>> On Mar 22, 2022, at 9:33 AM, Tom Lane  wrote:
>>> As a quick-n-dirty fix to avoid reverting the entire test module,
>>> perhaps just delete this error check for now.
>> Ok, done as you suggest:
> I only suggested removing the error check in _PG_init, not
> changing the way the test works.
>
>   



Mark and I discussed this offline, and decided there was no requirement
for the module to be preloaded. Do you have a different opinion?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 13:08, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 3/22/22 12:58, Tom Lane wrote:
>>> I only suggested removing the error check in _PG_init, not
>>> changing the way the test works.
>> Mark and I discussed this offline, and decided there was no requirement
>> for the module to be preloaded. Do you have a different opinion?
> No, I was actually about to make the same point: it seems to me there
> are arguable use-cases for loading it shared, loading it per-session
> (perhaps via ALTER USER SET or ALTER DATABASE SET to target particular
> users/DBs), or even manually LOADing it.  So the module code should
> not be prejudging how it's used.
>
> On reflection, I withdraw my complaint about changing the way the
> test script loads the module.  Getting rid of the need for a custom
> .conf file simplifies the test module, and that seems good.
> So I'm on board with Mark's patch now.
>
>   



OK, I have pushed that.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 14:01, Tom Lane wrote:
> Andrew Dunstan  writes:
>> OK, I have pushed that.
> It seems like you could remove the NO_INSTALLCHECK restriction
> too.  You already removed the comment defending it, and it
> seems to work fine as an installcheck now if I remove that
> locally.
>
> Other nitpicks:
>
> * the IsParallelWorker test could use a comment
>
> * I notice a typo "permisisons" in test_oat_hooks.sql
>
>       



Fixed


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 3/22/22 11:27, Andrew Dunstan wrote:
> On 3/22/22 10:53, Alvaro Herrera wrote:
>> On 2022-Mar-22, Andrew Dunstan wrote:
>>
>>> I'm planning on pushing the functions patch set this week and json-table
>>> next week.
>> I think it'd be a good idea to push the patches one by one and let a day
>> between each.  That would make it easier to chase buildfarm issues
>> individually, and make sure they are fixed before the next step.
>> Each patch in each series is already big enough.
>
>
> OK, can do it that way. First one will be later today then.
>
>

OK, pushed the first of the functions patches. That means the cfbot will
break on these two CF items, but it's way too much trouble to have to
remake the patch sets every day, so we can just live without the cfbot
on these for a bit. I will of course test before committing and fix any
bitrot.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: functions

[Andrew Dunstan]

On 3/5/22 09:39, Andrew Dunstan wrote:
>
> here's a new set of patches, omitting the GUC patch and with the
> beginnings of some message cleanup - there's more work to do there.
>
>
>
> This patchset restores the RETURNING clause for JSON() and JSON_SCALAR()
> but without the GUC
>
>


I have committed the first of these. That will break the cfbot for this
and the json_table patches. The remainder should be committed in the
following days.


cheers


andew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 18:18, Tom Lane wrote:
> Andrew Dunstan  writes:
>> Fixed
> Now that we got past the hard failures, we can see that the test
> falls over with (some?) non-default encodings, as for instance
> here:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2022-03-22%2020%3A23%3A13
>
> I can replicate that by running the test under LANG=en_US.iso885915.
> What I think is happening is:
>
> (1) Rather unwisely, the relevant InvokeNamespaceSearchHook calls
> appear in recomputeNamespacePath.  That means that their timing
> depends heavily on accidents of caching.
>
> (2) If we decide that we need an encoding conversion to talk to
> the client, there'll be a lookup for the conversion function
> early during session startup.  That will cause the namespace
> search path to get computed then, before the test module has been
> loaded and certainly before the audit GUC has been turned on.
>
> (3) At the point where the test expects some audit notices
> to come out, nothing happens because the search path is
> already validated.
>
> I'm inclined to think that (1) is a seriously bad idea,
> not only because of this instability, but because
>
> (a) the namespace cache logic is unlikely to cause the search-path
> cache to get invalidated when something happens that might cause an
> OAT hook to wish to change its decision, and
>
> (b) this placement means that the hook is invoked during cache loading
> operations that are likely to be super-sensitive to any additional
> catalog accesses a hook might wish to do.  (I await the results of the
> CLOBBER_CACHE_ALWAYS animals with trepidation.)
>
> Now, if our attitude to the OAT hooks is that we are going to
> sprinkle some at random and whether they are useful is someone
> else's problem, then maybe these are not interesting concerns.


So this was a pre-existing problem that the test has exposed? I don't
think we can just say "you deal with it", and if I understand you right
you don't think that either.

I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
temporarily.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: functions

[Andrew Dunstan]

On 3/22/22 18:31, Tom Lane wrote:
> I wrote:
>> That patch is 0-for-three on my buildfarm animals.
> ... the reason being that they use -Werror, and this patch spews
> a bunch of warnings.  This is not acceptable, especially not in
> the middle of a CF when other people are trying to get work done.
> Please revert.
>
>   



reverted.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 20:07, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 3/22/22 18:18, Tom Lane wrote:
>>> Now, if our attitude to the OAT hooks is that we are going to
>>> sprinkle some at random and whether they are useful is someone
>>> else's problem, then maybe these are not interesting concerns.
>> So this was a pre-existing problem that the test has exposed? I don't
>> think we can just say "you deal with it", and if I understand you right
>> you don't think that either.
> Yeah, my point exactly: the placement of those hooks needs to be rethought.
> I'm guessing what we ought to do is let the cached namespace OID list
> get built without interference, and then allow the OAT hook to filter
> it when it's read.
>
>> I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
>> temporarily.
> I was able to reproduce it under "make check" as long as I had
> LANG set to one of the troublesome values, so I'm not real sure
> that that'll be enough.
>
>   


The buildfarm only runs installcheck under different locales/encodings.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: cpluspluscheck vs ICU

[Andrew Dunstan]

On 3/22/22 22:23, Andres Freund wrote:
> Hi,
>
> On 2022-03-22 17:20:24 -0700, Andres Freund wrote:
>> I was about to propose adding headerscheck / cpluspluscheck to the CI file so
>> that cfbot can catch future issues.
> The attached patch does so, with ICU disabled to avoid the problems discussed
> in the thread. Example run:
> https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0
>
> Unless somebody sees a reason not to, I'm planning to commit this soon.
>

That only helps when running the CI/cfbot setup. Fixing it for other
(manual or buildfarm) users would be nice. Luckily crake isn't building
with ICU.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

clean up test_rls_hooks module

[Andrew Dunstan]

Over at [1] Mark Dilger got led astray because he used
src/test/modules/test_rls_hooks as a template for his new test module.
It looks like that module was created with some over eager copying of
another test module, but it has a couple of things wrong with it.

. it isn't an extension, so the Makefile shouldn't have an EXTENSION
entry, and there shouldn't be a .control file

. it doesn't need to be preloaded so there is no requirement for a
special config, nor for corresponding REGRESS_OPTS and NO_INSTALLCHECK
lines in the Makefile.

Here's a patch to fix those things.


cheers


andrew


[1]
https://postgr.es/m/47f87a0e-c0e5-43a6-89f6-d403f2b45...@enterprisedb.com

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 21f316d29881c3294f8d0b1bc2f0dde8eec38c2d Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Wed, 23 Mar 2022 08:49:07 -0400
Subject: [PATCH] fix test_rls_hooks

---
 src/test/modules/test_rls_hooks/Makefile | 12 ++--
 src/test/modules/test_rls_hooks/rls_hooks.conf   |  1 -
 .../modules/test_rls_hooks/test_rls_hooks.control|  4 
 3 files changed, 2 insertions(+), 15 deletions(-)
 delete mode 100644 src/test/modules/test_rls_hooks/rls_hooks.conf
 delete mode 100644 src/test/modules/test_rls_hooks/test_rls_hooks.control

diff --git a/src/test/modules/test_rls_hooks/Makefile b/src/test/modules/test_rls_hooks/Makefile
index a4f7d855c0..519eb9513d 100644
--- a/src/test/modules/test_rls_hooks/Makefile
+++ b/src/test/modules/test_rls_hooks/Makefile
@@ -1,19 +1,11 @@
 # src/test/modules/test_rls_hooks/Makefile
 
 MODULE_big = test_rls_hooks
-OBJS = \
-	$(WIN32RES) \
-	test_rls_hooks.o
-PGFILEDESC = "test_rls_hooks - example use of RLS hooks"
+OBJS = test_rls_hooks.o $(WIN32RES)
 
-EXTENSION = test_rls_hooks
-# DATA = test_rls_hooks--1.0.sql
+PGFILEDESC = "test_rls_hooks - example use of RLS hooks"
 
 REGRESS = test_rls_hooks
-REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
-# Disabled because these tests require "shared_preload_libraries=test_rls_hooks",
-# which typical installcheck users do not have (e.g. buildfarm clients).
-NO_INSTALLCHECK = 1
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/src/test/modules/test_rls_hooks/rls_hooks.conf b/src/test/modules/test_rls_hooks/rls_hooks.conf
deleted file mode 100644
index a522c0e550..00
--- a/src/test/modules/test_rls_hooks/rls_hooks.conf
+++ /dev/null
@@ -1 +0,0 @@
-shared_preload_libraries = test_rls_hooks
diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.control b/src/test/modules/test_rls_hooks/test_rls_hooks.control
deleted file mode 100644
index 9f9f13f76c..00
--- a/src/test/modules/test_rls_hooks/test_rls_hooks.control
+++ /dev/null
@@ -1,4 +0,0 @@
-comment = 'Test code for RLS hooks'
-default_version = '1.0'
-module_pathname = '$libdir/test_rls_hooks'
-relocatable = true
-- 
2.25.1

Re: New Object Access Type hooks

[Andrew Dunstan]

On 3/22/22 20:11, Andrew Dunstan wrote:
> On 3/22/22 20:07, Tom Lane wrote:
>> Andrew Dunstan  writes:
>>> On 3/22/22 18:18, Tom Lane wrote:
>>>> Now, if our attitude to the OAT hooks is that we are going to
>>>> sprinkle some at random and whether they are useful is someone
>>>> else's problem, then maybe these are not interesting concerns.
>>> So this was a pre-existing problem that the test has exposed? I don't
>>> think we can just say "you deal with it", and if I understand you right
>>> you don't think that either.
>> Yeah, my point exactly: the placement of those hooks needs to be rethought.
>> I'm guessing what we ought to do is let the cached namespace OID list
>> get built without interference, and then allow the OAT hook to filter
>> it when it's read.
>>
>>> I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
>>> temporarily.
>> I was able to reproduce it under "make check" as long as I had
>> LANG set to one of the troublesome values, so I'm not real sure
>> that that'll be enough.
>>
>>  
>
> The buildfarm only runs installcheck under different locales/encodings.


But you're right about the non-installcheck cases. fairywren had that
issue. I have committed a (tested) fix for those too to force
NO_LOCALE/UTF8.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: functions

[Andrew Dunstan]

On 3/23/22 08:24, Justin Pryzby wrote:
> At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
> per COPY_PARSE_PLAN_TREES.
>
> +ERROR:  unrecognized node type: 157



I just tried to reproduce this and was unable to.  Ubuntu 20.04, gcc 9.4.0.


the build was done with CPPFLAGS=-DCOPY_PARSE_PLAN_TREES. To be on the
safe side I took ccache out of the equation.


Can you give me any more details?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: SQL/JSON: functions

[Andrew Dunstan]

On 3/23/22 15:49, Andrew Dunstan wrote:
> On 3/23/22 08:24, Justin Pryzby wrote:
>> At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
>> per COPY_PARSE_PLAN_TREES.
>>
>> +ERROR:  unrecognized node type: 157
>
>
> I just tried to reproduce this and was unable to.  Ubuntu 20.04, gcc 9.4.0.
>
>
> the build was done with CPPFLAGS=-DCOPY_PARSE_PLAN_TREES. To be on the
> safe side I took ccache out of the equation.
>
>
> Can you give me any more details?
>
>

I have reproduced similar errors from patch 4 in the series.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: proposal: unescape_text function

[Andrew Dunstan]

On 11/30/20 8:14 AM, Peter Eisentraut wrote:
> On 2020-11-29 18:36, Pavel Stehule wrote:
>>
>>     I don't really get the point of this function.  There is AFAICT no
>>     function to produce this escaped format, and it's not a recognized
>>     interchange format.  So under what circumstances would one need to
>>     use this?
>>
>>
>> Some corporate data can be in CSV format with escaped unicode
>> characters. Without this function it is not possible to decode these
>> files without external application.
>
> I would like some supporting documentation on this.  So far we only
> have one stackoverflow question, and then this implementation, and
> they are not even the same format.  My worry is that if there is not
> precise specification, then people are going to want to add things in
> the future, and there will be no way to analyze such requests in a
> principled way.
>
>
>


Also, should this be an extension? I'm dubious about including such
marginal uses in the core code unless there's a really good case for it.


cheers


andrew

Re: proposal: unescape_text function

[Andrew Dunstan]

On 12/2/20 12:48 AM, Pavel Stehule wrote:
>
>
> st 2. 12. 2020 v 0:05 odesílatel Andrew Dunstan  <mailto:and...@dunslane.net>> napsal:
>
>
> On 11/30/20 8:14 AM, Peter Eisentraut wrote:
> > On 2020-11-29 18:36, Pavel Stehule wrote:
> >>
> >>     I don't really get the point of this function.  There is
> AFAICT no
> >>     function to produce this escaped format, and it's not a
> recognized
> >>     interchange format.  So under what circumstances would one
> need to
> >>     use this?
> >>
> >>
> >> Some corporate data can be in CSV format with escaped unicode
> >> characters. Without this function it is not possible to decode
> these
> >> files without external application.
> >
> > I would like some supporting documentation on this.  So far we only
> > have one stackoverflow question, and then this implementation, and
> > they are not even the same format.  My worry is that if there is not
> > precise specification, then people are going to want to add
> things in
> > the future, and there will be no way to analyze such requests in a
> > principled way.
> >
> >
> >
>
>
> Also, should this be an extension? I'm dubious about including such
> marginal uses in the core code unless there's a really good case
> for it.
>
>
>
[...]
> 3. there are new disadvantages of extensions in current DBaaS times.
> Until the extension is not directly accepted by a cloud provider, then
> the extension is not available for users. The acceptance of extensions
> is not too agile - so moving this code to extension doesn't solve this
> problem. Without DBaaS the implementation of this feature as the
> extensions can be good enough.
>
>

That argument can apply to any extension someone wants to use. If your
DBaaS provider doesn't support some extension you need to lobby them or
find another that does support it, rather than try to put it in core
code. Some extensions, such as untrusted PLs,  will naturally almost
never be supported by DBaaS providers because they are inherently
unsafe. That's not the case here.


cheers


andrew

Re: Commitfest 2020-11 is closed

[Andrew Dunstan]

On 12/2/20 5:13 PM, Thomas Munro wrote:
>
> I'm experimenting with Github's built in CI.  All other ideas welcome.



I'd look very closely at gitlab.


cheers


andrew

Re: Commitfest 2020-11 is closed

[Andrew Dunstan]

On 12/3/20 4:54 AM, Anastasia Lubennikova wrote:
> On 02.12.2020 23:59, Tom Lane wrote:
>> Anastasia Lubennikova  writes:
>>> Commitfest 2020-11 is officially closed now.
>>> Many thanks to everyone who participated by posting patches, reviewing
>>> them, committing and sharing ideas in discussions!
>> Thanks for all the hard work!
>>
>>> Today, me and Georgios will move the remaining items to the next CF or
>>> return them with feedback. We're planning to leave Ready For Committer
>>> till the end of the week, to make them more visible and let them get
>>> the
>>> attention they deserve.
>> This is actually a bit problematic, because now the cfbot is ignoring
>> those patches (or if it's not, I don't know where it's displaying the
>> results).  Please go ahead and move the remaining open patches, or
>> else re-open the CF if that's possible.
>>
>>     regards, tom lane
>
> Oh, I wasn't aware of that. Thank you for the reminder.
>
> Now all patches are moved to the next CF.
>

Maybe this needs to be added to the instructions for CF managers so the
workflow is clear.


cheers


andrew

Re: Github Actions (CI)

[Andrew Dunstan]

On 12/3/20 1:33 AM, Thomas Munro wrote:
> Hi hackers,
>
> I'm looking for more horsepower for testing commitfest entries
> automatically, and today I tried out $SUBJECT.  The attached is a
> rudimentary first attempt, for show-and-tell.  If you have a Github
> account, you just have to push it to a branch there and look at the
> Actions tab on the web page for the results.  
>

Awesome. That's a pretty big bang for the buck.


cheers


andrew

Re: Change definitions of bitmap flags to bit-shifting style

[Andrew Dunstan]

On 12/6/20 11:44 AM, James Coleman wrote:
> On Sun, Dec 6, 2020 at 1:25 AM Michael Paquier  > wrote:
>
> On Sat, Dec 05, 2020 at 10:31:09PM -0300, Alvaro Herrera wrote:
> > The hexadecimal representation is more natural to me than
> bit-shifting,
> > so I would prefer to use that style too.  But maybe I'm trained
> to it
> > because of looking at t_infomask symbols constantly.
>
> If we are going to change all that, hexa style sounds good to me too.
> Would it be worth an addition to the docs, say in [1] to tell that
> this is a preferred style?
>
> [1]: https://www.postgresql.org/docs/devel/source-conventions.html
> ?
> --
> Michael
>
>
>
> In my view the bit shifting approach makes it more obvious a single
> bit is being set, but on the other hand the hex approach makes it
> easier to compare in debugging. 
>
> I’m not really sure which to prefer, though I think I would have
> leaned slightly towards the former. 
>
>

Perhaps we should put one style or the other in a comment. I take Tom's
point, but after the number of bits shifted gets above some number I
have trouble remembering which bit it is, and while of course I can work
it out, it can be a very minor nuisance.


cheers


andrew

Re: TAP tests aren't using the magic words for Windows file access

[Andrew Dunstan]

On 12/15/20 12:05 AM, r.zhar...@postgrespro.ru wrote:
> Hello hackers,
>
> Are there any plans to backport the patch to earlier versions
> of the Postgres?
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=114541d58e5970e51b78b77b65de16210beaab43
>
>
> We rarely see the issue with the pg_ctl/004_logrotate test on
> the REL_12_STABLE branch. On my notebook I can easily reproduce
> the "Permission denied at src/test/perl/TestLib.pm line 259"
> error with the small change below. But the same test on the
> 13th version and the 12th version with the TestLib patch does
> not fail.
>
> diff --git a/src/bin/pg_ctl/t/004_logrotate.pl
> b/src/bin/pg_ctl/t/004_logrotate.pl 
>    
> index bc39abd23e4..e49e159bc84
> 100644   
> 
>   ---
> a/src/bin/pg_ctl/t/004_logrotate.pl 
> 
>   +++
> b/src/bin/pg_ctl/t/004_logrotate.pl 
> 
>   @@ -72,7 +72,7 @@ for (my
> $attempts = 0; $attempts < $max_attempts;
> $attempts++)  
> 
> 
> {  
> 
> 
>  $new_current_logfiles = slurp_file($node->data_dir .
> '/current_logfiles');   
>   last
> if $new_current_logfiles ne
> $current_logfiles;  
> 
>  -  
> usleep(100_000);   
> 
> 
>   +  
> usleep(1);
> 
> 
> }  
> 
> 
> 
> 
>    
> note "now current_logfiles = $new_current_logfiles";
>
>
>


Oops, looks like that slipped off my radar somehow, I'll see about
backpatching it right away.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: \gsetenv

[Andrew Dunstan]

On 12/16/20 10:54 PM, David Fetter wrote:
>
>> Besides which, you haven't bothered with even one word of positive
>> justification.  What's the non-hazardous use case?
> Thanks for asking, and my apologies for not including it.
>
> I ran into a situation where we sometimes got a very heavily loaded
> and also well-protected PostgreSQL server. At times, just getting a
> shell on it could take a few tries. To mitigate situations like that,
> I used a method that's a long way from new, abstruse, or secret: have
> psql open in a long-lasting tmux or screen session. It could both
> access the database at a time when getting a new connection would be
> somewhere between difficult and impossible.  The bit that's unlikely
> to be new was when I noticed that it could also shell out
> and send information off to other places, but only when I put together
> a pretty baroque procedure that involved using combinations of \gset,
> \o, and \!. All of the same things \gsetenv could do were doable with
> those, just less convenient, so I drafted up a patch in the hope that
> fewer others would find themselves jumping through the hoops I did to
> get that set up.


Does this help?


andrew=# select 'abc'::text as foo \gset
andrew=# \setenv FOO :foo
andrew=# \! echo $FOO
abc
andrew=#


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

multi-install PostgresNode

[Andrew Dunstan]

I've been giving some thought to $subject. The initial impetus is the
promise I made to assist with testing of clients built with NSS against
servers built with openssl, and vice versa.

I've already generalized the process of saving binaries by the buildfarm
client. And we could proceed with a purely bespoke module for testing
the SSL components, as we already do for testing cross-version
pg_upgrade. But it struck me that it might be better to leverage our
existing investment in TAP tests. So I came up with the idea of creating
a child module of PostgresNode.pm, which would set the PATH and other
variables appropriately at the start of each method and restore them on
method exit. So then we could have things like:

$openssl_node->start;
my $connstr = $openssl_node->connstr;
$nss_node->psql($connstr, ...);
  

To use this a TAP test would need to know two (or more) install paths
for the various nodes, presumably set in environment variables much as
we do now for things like TESTDIR. So for the above example, the TAP
test could set things up with:

my

$openssl_node=PostgresNodePath::get_new_node($ENV{OPENSSL_POSTGRES_INSTALL_PATH},'openssl');
my

$nss_node=PostgresNodePath::get_new_node($ENV{NSS_POSTGRES_INSTALL_PATH},'nss');

Other possible uses would be things like cross-version testing of
pg_dump (How do we know we haven't broken anything in dumping very old
versions?).

The proposed module would look something like this:

package PostgresNodePath;

use strict;
use warnings;

use parent PostgresNode;

use Exporter qw(import);
our @EXPORT = qw(get_new_node);

sub get_new_node
{
    my $installpath= shift;
    my $node = PostgresNode::get_new_node(@_);
    bless $node; # re-bless into current class
    $node->{_installpath} = $installpath;
    return $node;
}

and then  for each class method in PostgresNode.pm we'd have an override
something like:

sub foo
{
    my $node=shift;
    my $inst = $node->{_installpath};
    local %ENV = %ENV;
    $ENV{PATH} = "$inst/bin:$ENV{PATH}";
    $ENV{LD_LIBRARY_PATH} = "$inst/lib:$ENV{LD_LIBRARY_PATH}";
    $node->SUPER::foo(@_);
}

There might be more elegant ways of doing this, but that's what I came
up with.

My main question is: do we want something like this in the core code
(presumably in src/test/perl), or is it not of sufficiently general
interest? If it's wanted I'll submit a patch, probably for the March CF,
but January if I manage to get my running shoes on. If not, I'll put it
in the buildfarm code, but then any TAP tests that want it will likewise
need to live there.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode

[Andrew Dunstan]

On 12/17/20 7:55 PM, Michael Paquier wrote:
> On Thu, Dec 17, 2020 at 04:37:54PM -0500, Andrew Dunstan wrote:
>> The proposed module would look something like this:
>>
>> [...]
>>
>> use parent PostgresNode;
>>
>> sub get_new_node
>> {
>>     my $installpath= shift;
>>     my $node = PostgresNode::get_new_node(@_);
>>     bless $node; # re-bless into current class
>>     $node->{_installpath} = $installpath;
>>     return $node;
>> }
> Passing down the installpath as argument and saving it within a
> PostgresNode or child class looks like the correct way of doing things
> to me.  This would require an extra routine to be able to get the
> install path from a node as _installpath would remain internal to the
> module file, right?  Shouldn't it be something that ought to be
> directly part of PostgresNode actually, where we could enforce the lib
> and bin paths to the output of pg_config if an _installpath is not
> provided by the caller?  In short, I am not sure that we need an extra
> module here.
>
>> and then  for each class method in PostgresNode.pm we'd have an override
>> something like:
>>
>> sub foo
>> {
>>     my $node=shift;
>>     my $inst = $node->{_installpath};
>>     local %ENV = %ENV;
>>     $ENV{PATH} = "$inst/bin:$ENV{PATH}";
>>     $ENV{LD_LIBRARY_PATH} = "$inst/lib:$ENV{LD_LIBRARY_PATH}";
>>     $node->SUPER::foo(@_);
>> }
>>
>> There might be more elegant ways of doing this, but that's what I came
>> up with.
> As long as it does not become necessary to pass down _installpath to
> all indidivual binary calls we have in PostgresNode or the extra
> module, this gets a +1 from me.  So, if I am getting that right, the
> key point is the use of local %ENV here to make sure that PATH and
> LD_LIBRARY_PATH are only enforced when it comes to calls within
> PostgresNode.pm, right?  That's an elegant solution.  This is
> something I have wanted for a long time for pg_upgrade to be able to
> get rid of its test.sh.
>
>> My main question is: do we want something like this in the core code
>> (presumably in src/test/perl), or is it not of sufficiently general
>> interest? If it's wanted I'll submit a patch, probably for the March CF,
>> but January if I manage to get my running shoes on. If not, I'll put it
>> in the buildfarm code, but then any TAP tests that want it will likewise
>> need to live there.
> This facility gives us the possibility to clean up the test code of
> pg_upgrade and move it to a TAP test, so I'd say that it is worth
> having in the core code in the long-term.


This turns out to be remarkably short, with the use of a little eval magic.

Give the attached, this test program works just fine:

#!/bin/perl
use PostgresNodePath;
$ENV{PG_REGRESS} =
'/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
print $node->info;
print $node->connstr(),"\n";
$node->init();

cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com



PostgresNodePath.pm
Description: Perl program

Re: how to find log

[Andrew Dunstan]

On 12/20/20 11:31 AM, Tom Lane wrote:
> Dmitry Markman  writes:
>> suppose I started the server with the following command
>> pg_ctl -D . . . start -l 
>> is there a way to get  later by sending some query to the 
>> server or
> No, the server has no way to know where its stdout/stderr were
> pointed to.  You might want to enable the syslogger output method
> (see logging_collector) to have something a bit more featureful.
>
> https://www.postgresql.org/docs/current/runtime-config-logging.html
>
>   



Alternatively, asking the OS in many cases will work, e.g. on my linux
machine:


ls -l /proc/{postmasterpid}/fd/1


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode

[Andrew Dunstan]

On 12/19/20 11:19 AM, Andrew Dunstan wrote:
>
>
> This turns out to be remarkably short, with the use of a little eval magic.
>
> Give the attached, this test program works just fine:
>
> #!/bin/perl
> use PostgresNodePath;
> $ENV{PG_REGRESS} =
> '/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
> my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
> print $node->info;
> print $node->connstr(),"\n";
>     $node->init();
>


This time with a typo removed.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com



PostgresNodePath.pm
Description: Perl program

Re: proposal - support tsv output format for psql

[Andrew Dunstan]

On 12/23/20 10:13 AM, Pavel Stehule wrote:
>
>
> After this second check, I think the new format is not necessary,
> because tsv in LO is not tsv, but it is cracked csv
>
>


I agree, I don't think this is a format we need to spent much time on.


If you set the quote to an unlikely character like ^A, the escape
character to '\', and the delimiter to TAB, COPY in CSV mode will give
you something pretty close in many cases. Embedded newlines will still
give you trouble.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Buildfarm's cross-version-upgrade tests

[Andrew Dunstan]

On 12/30/20 3:42 PM, Tom Lane wrote:
> I broke $SUBJECT again today by removing regress_putenv from
> regress.so.  I don't really want to put it back, but that means
> we need a way to drop the corresponding SQL function from the
> old version's regression database before attempting an upgrade.
>
> Since I'd just seen Noah's commit 52202bb39 go by, I tried
> modifying src/bin/pg_upgrade/test.sh to do the drop, but that
> isn't helping.  I recall now from prior discussions that we
> have to modify code that's embedded in the buildfarm client
> script to make this go.  (I wonder what test scenario Noah had
> in mind, exactly.)
>
> Realizing that this has happened before and will happen again,
> I'm now wondering if we can't devise a fixup method that is
> controllable by committers and doesn't require a buildfarm
> client rollout to update.  I'm thinking maybe we could extract
> the fixup logic from test.sh into a standalone SQL script that's
> part of the regular source tree so it can be updated by committers,
> and then both test.sh and the buildfarm script could invoke it.
> Maybe that won't work for some reason, but I'd sure like to find
> some way of handling this without making you get involved every time.
>
>   



Well, it's not hugely onerous, although it does seem to have happened a
bit more lately than once it used to. Basically, there are two sets of
adjustments. First there are the things we do when we're saving a
cluster for later upgrade testing. These can be found at lines 238 - 300
of the module (see
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm>).
Then, when it comes time to do an upgrade test, we make a copy of the
cluster and make further adjustments depending on the target version as
well as the source version. See lines 363 - 496. Some of this is a bit
more complicated because we test all the way back to 9.2, even though
it's past EOL.

It seems reasonable to want to institutionalize this knowledge. I'll see
if I can extract it into one or two perl scripts suitable for inclusion
in core code.

I'll try to fix the test for the latest breakage shortly.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Buildfarm's cross-version-upgrade tests

[Andrew Dunstan]

On 12/30/20 4:28 PM, Andrew Dunstan wrote:
>
> I'll try to fix the test for the latest breakage shortly.



See
<https://github.com/PGBuildFarm/client-code/commit/fa86d0b1bc7a8d7b9f15b1da8b8e43f4d3a08e2b>


I'm going to get a new release out before next Thursday come hell or
high water.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Safety/validity of resetting permissions by updating system tables

[Andrew Dunstan]

On 1/1/21 11:44 AM, Tom Lane wrote:
> Isaac Morland  writes:
>> Is it safe and valid to reset to default permissions by doing
>> UPDATE pg_namespace/pg_class/pg_type/pg_proc
>> SET nspacl/relacl/typacl/proacl = NULL WHERE ... to accomplish this?
> Not terribly; the main objection is you'd fail to update pg_shdepend.



And apart from that I'm generally resistant to anything that requires
direct manipulation of the catalog. One of many reasons is that there is
no guarantee that it will have the same shape in the next release. I
normally encourage people strongly to look for other solutions.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Safety/validity of resetting permissions by updating system tables

[Andrew Dunstan]

On 1/4/21 11:15 AM, Isaac Morland wrote:
> On Mon, 4 Jan 2021 at 10:12, Andrew Dunstan  <mailto:and...@dunslane.net>> wrote:
>
>
> On 1/1/21 11:44 AM, Tom Lane wrote:
> > Isaac Morland  <mailto:isaac.morl...@gmail.com>> writes:
> >> Is it safe and valid to reset to default permissions by doing
> >> UPDATE pg_namespace/pg_class/pg_type/pg_proc
> >> SET nspacl/relacl/typacl/proacl = NULL WHERE ... to accomplish
> this?
> > Not terribly; the main objection is you'd fail to update
> pg_shdepend.
>
> And apart from that I'm generally resistant to anything that requires
> direct manipulation of the catalog. One of many reasons is that
> there is
> no guarantee that it will have the same shape in the next release. I
> normally encourage people strongly to look for other solutions.
>
>
> So am I. That's why I asked before proceeding.
>
> As far as I can tell, it is not possible to fully reset permissions
> using GRANT/REVOKE even querying the system tables to figure out which
> permissions exist; the closest one can get is to set explicit
> (non-NULL) acls that have the same effect as the default (NULL) acls;
> and doing so requires duplicating the logic used within the system to
> determine the permissions that apply to an object with a blank (NULL) acl.



I think there is probably a good case for some sort of "from scratch"
option on GRANT.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Context diffs

[Andrew Dunstan]

On 1/4/21 5:21 PM, Thomas Munro wrote:
> On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian  wrote:
>> *  "git apply" and "git am" can't process context diffs (they throw an
>>error once a context-like section of the diff is hit; simple
>>adding/removing lines in a block works)
>>
>> *  the commit-fest doesn't recognized context diff attachments as
>> patches:
>>
>> https://commitfest.postgresql.org/31/2912/
>>
>> *  cfbot can't process file renames/add/delete from context diffs
> For the record, cfbot just uses plain old GNU patch, because that
> seems to accept nearly everything that anyone posts here (after a step
> that tries to unpack tarballs etc).  Several people have suggested I
> change it to use git apply instead (IIRC it works better for patches
> containing binary files such as cryptographic keys?), but then it
> wouldn't accept ye olde context diffs.
>
>

I would try 'git apply' and fall back on 'patch' if it fails.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Cirrus CI (Windows help wanted)

[Andrew Dunstan]

On 1/4/21 10:58 PM, Thomas Munro wrote:
> Hi,
>
> My new favourite CI is Cirrus CI, because it has 4 operating systems,
> generous enough quotas to handle 250+ branches in a single account,
> and public build/test log URLs.  I flipped cfbot.cputube.org (mostly)
> over to that and it seems to work well so far -- fingers crossed.
> I've also been using it for my own development branches that involve
> some systems hacking-heavy work that uses different kernel interfaces
> on all 4 of those OSes.
>
> There's one thing I'm stuck on, though: Windows.  If anyone wants to
> help figure out how to get PostgreSQL to build on Cirrus's Windows,
> I'd be very interested.  To play with this stuff, you need a public
> Github repo, and you need to add Cirrus CI from "Marketplace" (it's
> free for public/open source), and then you add a .cirrus.yml file such
> as https://github.com/macdice/cfbot/blob/master/cirrus/.cirrus.yml to
> the top level of a PostgreSQL branch.  When you push, you should see
> build results on the Github web UI.
>
> For a similar example that works on Windows on another CI, see
> https://github.com/macdice/cfbot/blob/master/appveyor/appveyor.yml
> (note that it also references a couple of other files; it'd be nice to
> be able to do that stuff without the need for separate files, possibly
> by using Power Shell).  That's what cfbot is using for Windows for
> now, which works really well, but it'd be nice to have more
> options/choices.  For another example of Windows builds working on
> another CI, see the Github Actions patch I posted earlier when I was
> considering that for cfbot[1].  I think what's different is that those
> other CIs have images with MSVC on them, but Cirrus wants you to
> figure out how to install the right toolchain yourself (and then, as a
> next step after it's actually working, it also provides a way to
> define what you want in a way that captures the resulting image using
> Docker voodoo, so that you get fast startup times).  Or something.
>
> [1] 
> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2By_SHVQcU3CPokmJxuHp1niebCjq4XzZizf8SR9ZdQRQ%40mail.gmail.com
>
>

Here's what I use for MS Tools in automated setups, which gives you all
you should need:

choco install -y --no-progress --limit-output
visualstudio2019-workload-vctools --package-parameters
"--includeOptional"

Your PATH adjustment should add this:

C:\Program Files (x86)\Microsoft Visual 
Studio\2019\BuildTools\MSBuild\Current\bin

There might be other environment settings needed - see drongo's config on the 
buildfarm. LMK how you get on.

Constructing an image where you don't have to do that every build would be 
super nice.


cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Context diffs

[Andrew Dunstan]

On 1/5/21 12:58 PM, Bruce Momjian wrote:
> On Tue, Jan  5, 2021 at 11:21:07AM +1300, Thomas Munro wrote:
>> On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian  wrote:
>>> *  "git apply" and "git am" can't process context diffs (they throw an
>>>error once a context-like section of the diff is hit; simple
>>>adding/removing lines in a block works)
>>>
>>> *  the commit-fest doesn't recognized context diff attachments as
>>> patches:
>>>
>>> https://commitfest.postgresql.org/31/2912/
>>>
>>> *  cfbot can't process file renames/add/delete from context diffs
>> For the record, cfbot just uses plain old GNU patch, because that
>> seems to accept nearly everything that anyone posts here (after a step
>> that tries to unpack tarballs etc).  Several people have suggested I
>> change it to use git apply instead (IIRC it works better for patches
>> containing binary files such as cryptographic keys?), but then it
>> wouldn't accept ye olde context diffs.
> Does Windows also use 'patch'?  I think I saw Windows behave differently
> for file additions.  Does the commit-fest app and cfbot both have the
> same criteria for recognizing attachments as patches?  I don't think
> they do.
>

Patch is available for windows both as a standard program under Msys and
as a native tool via "choco install patch". But unless you're developing
on Windows you would not need to bother about that at all. And if you
are developing on Windows, just produce your patches using "git diff" or
"git format-patch" just like on Unix.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Cirrus CI (Windows help wanted)

[Andrew Dunstan]

On 1/5/21 7:18 AM, Andrew Dunstan wrote:
> On 1/4/21 10:58 PM, Thomas Munro wrote:
>> Hi,
>>
>> My new favourite CI is Cirrus CI, because it has 4 operating systems,
>> generous enough quotas to handle 250+ branches in a single account,
>> and public build/test log URLs.  I flipped cfbot.cputube.org (mostly)
>> over to that and it seems to work well so far -- fingers crossed.
>> I've also been using it for my own development branches that involve
>> some systems hacking-heavy work that uses different kernel interfaces
>> on all 4 of those OSes.
>>
>> There's one thing I'm stuck on, though: Windows.  If anyone wants to
>> help figure out how to get PostgreSQL to build on Cirrus's Windows,
>> I'd be very interested.  To play with this stuff, you need a public
>> Github repo, and you need to add Cirrus CI from "Marketplace" (it's
>> free for public/open source), and then you add a .cirrus.yml file such
>> as https://github.com/macdice/cfbot/blob/master/cirrus/.cirrus.yml to
>> the top level of a PostgreSQL branch.  When you push, you should see
>> build results on the Github web UI.
>>
>> For a similar example that works on Windows on another CI, see
>> https://github.com/macdice/cfbot/blob/master/appveyor/appveyor.yml
>> (note that it also references a couple of other files; it'd be nice to
>> be able to do that stuff without the need for separate files, possibly
>> by using Power Shell).  That's what cfbot is using for Windows for
>> now, which works really well, but it'd be nice to have more
>> options/choices.  For another example of Windows builds working on
>> another CI, see the Github Actions patch I posted earlier when I was
>> considering that for cfbot[1].  I think what's different is that those
>> other CIs have images with MSVC on them, but Cirrus wants you to
>> figure out how to install the right toolchain yourself (and then, as a
>> next step after it's actually working, it also provides a way to
>> define what you want in a way that captures the resulting image using
>> Docker voodoo, so that you get fast startup times).  Or something.
>>
>> [1] 
>> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2By_SHVQcU3CPokmJxuHp1niebCjq4XzZizf8SR9ZdQRQ%40mail.gmail.com
>>
>>
> Here's what I use for MS Tools in automated setups, which gives you all
> you should need:
>
> choco install -y --no-progress --limit-output
> visualstudio2019-workload-vctools --package-parameters
> "--includeOptional"
>
> Your PATH adjustment should add this:
>
> C:\Program Files (x86)\Microsoft Visual 
> Studio\2019\BuildTools\MSBuild\Current\bin
>
> There might be other environment settings needed - see drongo's config on the 
> buildfarm. LMK how you get on.
>
> Constructing an image where you don't have to do that every build would be 
> super nice.
>
>

OK, I have dug into this quite a bit. The way cirrus works is in fact
somewhat fragile. Anyway, here are the highlights:


The image you're using comes with these choco packages pre-installed:


Chocolatey v0.10.15
7zip 19.0
7zip.install 19.0
chocolatey 0.10.15
chocolatey-core.extension 1.3.5.1
chocolatey-dotnetfx.extension 1.0.1
chocolatey-visualstudio.extension 1.8.1
chocolatey-windowsupdate.extension 1.0.4
cmake 3.19.1
cmake.install 3.19.1
dotnetfx 4.8.0.20190930
git 2.29.2.3
git.install 2.29.2.3
KB2919355 1.0.20160915
KB2919442 1.0.20160915
KB2999226 1.0.20181019
KB3033929 1.0.5
KB3035131 1.0.3
mingw 8.1.0
vcredist140 14.28.29325.2
visualstudio-installer 2.0.1
visualstudio2019-workload-vctools 1.0.0
visualstudio2019buildtools 16.8.2.0


However, sadly the vctools package above isn't installed with all its
optional packages, so some crucial things are missing. I cured that by
forcing a reinstall with the optional components enabled. Sadly, that
takes a huge amount of time, over 20 minutes. We either need to find an
image where this isn't necessary or find out how to make one to use,
assuming that's possible. Or maybe we can ask cirrus to modify their
buildscripts just a tad to include the required parameter.

The other issue I ran into was one with the ActivePerl install script. I
got around that by installing StrawberryPerl instead. It should work
fine I believe.

So here is the .cirrus.yml file I came up with. The last line fails as
expected because I have been testing in an environment without postgres
sources, but other than that it works. Note that we use VS's vcvarsall
script to set the environment properly for us.

task:
  name: Windows
  wi

Re: Cirrus CI (Windows help wanted)

[Andrew Dunstan]

On 1/5/21 11:19 PM, Thomas Munro wrote:
>
> Thanks!  I hacked on this a bit more and got as far as:
>
> C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>call
> perl buildsetup.pl
> Unable to determine Visual Studio version: The nmake version could not
> be determined. at src/tools/msvc/Mkvcbuild.pm line 92.
>
> That's from https://cirrus-ci.com/task/5842523031601152.  I guess PATH
> is wrong or nmake it not present, but it's so painful to do a test
> cycle that I gave up for today...


Hmm, weird. I'll play some more tomorrow and see what I can find.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Cirrus CI (Windows help wanted)

[Andrew Dunstan]

On 1/6/21 12:36 AM, Andrew Dunstan wrote:
> On 1/5/21 11:19 PM, Thomas Munro wrote:
>> Thanks!  I hacked on this a bit more and got as far as:
>>
>> C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>call
>> perl buildsetup.pl
>> Unable to determine Visual Studio version: The nmake version could not
>> be determined. at src/tools/msvc/Mkvcbuild.pm line 92.
>>
>> That's from https://cirrus-ci.com/task/5842523031601152.  I guess PATH
>> is wrong or nmake it not present, but it's so painful to do a test
>> cycle that I gave up for today...
>
> Hmm, weird. I'll play some more tomorrow and see what I can find.


I have it building and testing ok, but it's horribly fragile. I doubt
this is acceptable for the cfbot, you'll get far to many spurious failures.


There are some build warnings we don't usually see. I haven't delved
into that.


See <https://cirrus-ci.com/task/4602736530423808>


The yml file is:


task:
  name: Windows
  windows_container:
    image: cirrusci/windowsservercore:cmake
  install_script:
    - choco feature disable --name=showDownloadProgress
    - choco install -y winflexbison diffutils
    - rename c:\ProgramData\chocolatey\bin\win_flex.exe flex.exe
    - rename c:\ProgramData\chocolatey\bin\win_bison.exe bison.exe
    - choco install -y strawberryperl
    - choco install --force -y visualstudio2019-workload-vctools 
--package-parameters "--includeOptional"
    - choco list --localonly
  build_script:
    - cd "c:/Program Files (x86)/Microsoft Visual 
Studio/2019/BuildTools/VC/Auxiliary/Build"
    - vcvarsall x64
    - echo on
    - cd C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build
    - set PATH=C:\strawberry\perl\bin;%PATH%
    - set
    - perl src/tools/msvc/mkvcbuild.pl
    - msbuild pgsql.sln
  test_script:
        - set PATH=C:\strawberry\perl\bin;%PATH%
    - set
    - perl src/tools/msvc/vcregress.pl check


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Cirrus CI (Windows help wanted)

[Andrew Dunstan]

On 1/5/21 5:48 PM, I wrote:
> However, sadly the vctools package above isn't installed with all its
> optional packages, so some crucial things are missing. I cured that by
> forcing a reinstall with the optional components enabled. Sadly, that
> takes a huge amount of time, over 20 minutes. We either need to find an
> image where this isn't necessary or find out how to make one to use,
> assuming that's possible. Or maybe we can ask cirrus to modify their
> buildscripts just a tad to include the required parameter.


For some unfathomable reason they actually removed this less than a
month ago:

<https://github.com/cirruslabs/docker-images-windows/commit/6777ec66b76747a88f61252f9027f70d23fcc4ce>

I have identified the specific missing component as
Microsoft.VisualStudio.Component.VC.CLI.Support - I will submit a PR to
add it back in.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: WIP: System Versioned Temporal Table

[Andrew Dunstan]

On 1/8/21 7:33 AM, Simon Riggs wrote:
>
> * What happens if you ask for a future time?
> It will give an inconsistent result as it scans, so we should refuse a
> query for time > current_timestamp.


That seems like a significant limitation. Can we fix it instead of
refusing the query?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: PG vs LLVM 12 on seawasp, next round

[Andrew Dunstan]

On 1/24/21 11:06 PM, Tom Lane wrote:
> Noah Misch  writes:
>> On Mon, Jan 18, 2021 at 09:29:53PM +0100, Fabien COELHO wrote:
>>> ... Last two months were a
>>> little overworked for me so I let slip quite a few things. If you want to
>>> disable the animal as Tom suggests, do as you want.
>> Perhaps he was suggesting that you (buildfarm owner) disable the cron job 
>> that
>> initiates new runs.  That's what I do when one of my animals needs my
>> intervention.
> Indeed.  I'm not sure there even is a provision to block an animal on the
> buildfarm-server side.  If there is, you'd have to request that it be
> manually undone after you get around to fixing the animal.  Frankly,
> if I were the BF admin, I would be in about as much hurry to do that
> as you've been to fix it.
>
>   


It's actually very easy, but it's something I usually reserve for people
who are very unresponsive to emails. As noted, disabling the crontab
entry on the client side is a preferable solution.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Trigger execution role

[Andrew Dunstan]

On 2/16/21 3:59 PM, Isaac Morland wrote:
> On Fri, 12 Feb 2021 at 12:58, Tom Lane  <mailto:t...@sss.pgh.pa.us>> wrote:
>
> Isaac Morland  <mailto:isaac.morl...@gmail.com>> writes:
> > I was trying to use triggers, and ran into something I hadn't
> realized
> > until now: triggers run, not as the owner of the table, but as
> the user who
> > is doing the insert/update/delete.
>
> If you don't want that, you can make the trigger function SECURITY
> DEFINER.  If we forced such behavior, there'd be no way to get the
> other behavior.
>
>
> I did think about SECURITY DEFINER, but that has at least a couple of
> significant downsides:
>
> - can't re-use the same generic trigger function for different table
> owners; would need to duplicate the function and use the one whose
> owner matches the table
> - other users could make the function a trigger for their tables and
> then invoke it unexpectedly (i.e., in a scenario I didn’t anticipate)
> - have to grant EXECUTE on the function to the same users that need
> permission to change the table contents
>
> In what scenarios is it needed for the trigger to run as the role
> doing the INSERT/UPDATE/DELETE? There are lots of scenarios where it
> doesn't matter — I can think of any number of constraint enforcement
> triggers that just compute a boolean and which could run as either —
> but I find it a lot easier to imagine a scenario where the table owner
> wants to do something when an INSERT/UPDATE/DELETE occurs than one in
> which the table owner wants to make sure the role changing the table
> does something.


One fairly obvious example is where the trigger is inserting audit data.
It needs to log the name of the user running the triggering statement
rather than the table owner.

TBH, I've used triggers very extensively over the years for a wide
variety of purposes and not found this to be a great issue.


>
> Additionally, with the present behaviour, what happens when I change a
> table's contents is completely unpredictable. A table could have a
> trigger on it which drops all my tables, to take an extreme example.
> If “I” am postgres then this could be problematic: it’s not safe for a
> privileged user to make changes to the contents of another role’s
> tables unless they are first verified to have no triggers on them (or,
> in theory, that the triggers are harmless, but I’ve been playing
> enough Baba is You lately to consider any judgement that the triggers
> are harmless to be worthless without a formally verified proof of same).


Well, that's true of any function no matter how it's invoked. If the
function is malicious it will do damage. If you suspect the database to
contain malicious trigger code, maybe disabling user triggers is in order.

Anyway, just speculating, but maybe there is a case for allowing running
a trigger as the table owner, as part of the trigger creation. Certainly
we're a very long way past the time when we could reasonably change the
default.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Finding cause of test fails on the cfbot site

[Andrew Dunstan]

On 2/17/21 11:06 AM, Tom Lane wrote:
> Peter Smith  writes:
>> I saw that one of our commitfest entries (32/2914) is recently
>> reporting a fail on the cfbot site [1]. I thought this was all ok a
>> few days ago.
>> ...
>> Is there any other detailed information available anywhere, e.g.
>> logs?, which might help us work out what was the cause of the test
>> failure?
> AFAIK the cfbot doesn't capture anything beyond the session typescript.
> However, this doesn't look that hard to reproduce locally ... have you
> tried, using similar configure options to what that cfbot run did?
> Once you did reproduce it, there'd be logs under
> contrib/test_decoding/tmp_check/.
>
>   



yeah. The cfbot runs check-world which makes it difficult for it to know
which log files to show when there's an error. That's a major part of
the reason the buildfarm runs a much finer grained set of steps.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Finding cause of test fails on the cfbot site

[Andrew Dunstan]

On 2/17/21 3:42 PM, Thomas Munro wrote:
> On Thu, Feb 18, 2021 at 9:18 AM Andrew Dunstan  wrote:
>> On 2/17/21 11:06 AM, Tom Lane wrote:
>>> Peter Smith  writes:
>>>> I saw that one of our commitfest entries (32/2914) is recently
>>>> reporting a fail on the cfbot site [1]. I thought this was all ok a
>>>> few days ago.
>>>> ...
>>>> Is there any other detailed information available anywhere, e.g.
>>>> logs?, which might help us work out what was the cause of the test
>>>> failure?
>>> AFAIK the cfbot doesn't capture anything beyond the session typescript.
>>> However, this doesn't look that hard to reproduce locally ... have you
>>> tried, using similar configure options to what that cfbot run did?
>>> Once you did reproduce it, there'd be logs under
>>> contrib/test_decoding/tmp_check/.
>> yeah. The cfbot runs check-world which makes it difficult for it to know
>> which log files to show when there's an error. That's a major part of
>> the reason the buildfarm runs a much finer grained set of steps.
> Yeah, it's hard to make it print out just the right logs without
> dumping so much stuff that it's hard to see the wood for the trees;
> perhaps if the Makefile had an option to dump relevant stuff for the
> specific tests that failed, or perhaps the buildfarm is already better
> at that and cfbot should just use the buildfarm client directly.  Hmm.
> Another idea would be to figure out how to make a tarball of all log
> files that you can download for inspection with better tools at home
> when things go wrong.  It would rapidly blow through the 1GB limit for
> stored "artefacts" on open source/community Cirrus accounts though, so
> we'd need to figure out how to manage retention carefully.


I did some thinking about this. How about if we have the make files and
the msvc build system create a well known file with the location(s) to
search for log files if there's an error. Each bit of testing could
overwrite this file before starting testing, and then tools like cfbot
would know where to look for files to report? To keep things clean for
other users the file would only be created if, say,
PG_NEED_ERROR_LOG_LOCATIONS is set. The well known location would be
something like "$(top_builddir)/error_log_locations.txt", and individual
Makefiles would have entries something like:,


override ERROR_LOG_LOCATIONS =
$(top_builddir)/contrib/test_decoding/tmp_check/log


If this seems like a good idea I can go and try to make that happen.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Finding cause of test fails on the cfbot site

[Andrew Dunstan]

On 2/18/21 11:01 AM, Andrew Dunstan wrote:
> On 2/17/21 3:42 PM, Thomas Munro wrote:
>> On Thu, Feb 18, 2021 at 9:18 AM Andrew Dunstan  wrote:
>>> On 2/17/21 11:06 AM, Tom Lane wrote:
>>>> Peter Smith  writes:
>>>>> I saw that one of our commitfest entries (32/2914) is recently
>>>>> reporting a fail on the cfbot site [1]. I thought this was all ok a
>>>>> few days ago.
>>>>> ...
>>>>> Is there any other detailed information available anywhere, e.g.
>>>>> logs?, which might help us work out what was the cause of the test
>>>>> failure?
>>>> AFAIK the cfbot doesn't capture anything beyond the session typescript.
>>>> However, this doesn't look that hard to reproduce locally ... have you
>>>> tried, using similar configure options to what that cfbot run did?
>>>> Once you did reproduce it, there'd be logs under
>>>> contrib/test_decoding/tmp_check/.
>>> yeah. The cfbot runs check-world which makes it difficult for it to know
>>> which log files to show when there's an error. That's a major part of
>>> the reason the buildfarm runs a much finer grained set of steps.
>> Yeah, it's hard to make it print out just the right logs without
>> dumping so much stuff that it's hard to see the wood for the trees;
>> perhaps if the Makefile had an option to dump relevant stuff for the
>> specific tests that failed, or perhaps the buildfarm is already better
>> at that and cfbot should just use the buildfarm client directly.  Hmm.
>> Another idea would be to figure out how to make a tarball of all log
>> files that you can download for inspection with better tools at home
>> when things go wrong.  It would rapidly blow through the 1GB limit for
>> stored "artefacts" on open source/community Cirrus accounts though, so
>> we'd need to figure out how to manage retention carefully.
>
> I did some thinking about this. How about if we have the make files and
> the msvc build system create a well known file with the location(s) to
> search for log files if there's an error. Each bit of testing could
> overwrite this file before starting testing, and then tools like cfbot
> would know where to look for files to report? To keep things clean for
> other users the file would only be created if, say,
> PG_NEED_ERROR_LOG_LOCATIONS is set. The well known location would be
> something like "$(top_builddir)/error_log_locations.txt", and individual
> Makefiles would have entries something like:,
>
>
> override ERROR_LOG_LOCATIONS =
> $(top_builddir)/contrib/test_decoding/tmp_check/log
>
>
> If this seems like a good idea I can go and try to make that happen.
>
>

here's a very small and simple (and possibly naive)  POC patch that
demonstrates this and seems to do the right thing.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 74b3a6acd2..a1339d6ec9 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -390,6 +390,15 @@ endif
 endif
 
 
+.PHONY: set_error_log_locations
+
+set_error_log_locations:
+ifdef PG_NEED_ERROR_LOCATIONS
+ifdef ERROR_LOG_LOCATIONS
+	echo "$(ERROR_LOG_LOCATIONS)" > $(top_builddir)/error_log_locations.txt
+endif
+endif
+
 # Testing
 
 # In much the same way as above, these rules ensure that we build a temp
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index d00881163c..c325d76b52 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -12,6 +12,8 @@
 PGFILEDESC = "psql - the PostgreSQL interactive terminal"
 PGAPPICON=win32
 
+override ERROR_LOG_LOCATIONS = $(abs_top_builddir)/$(subdir)/tmp_check/log
+
 subdir = src/bin/psql
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global
@@ -83,7 +85,7 @@ clean distclean:
 maintainer-clean: distclean
 	rm -f sql_help.h sql_help.c psqlscanslash.c
 
-check:
+check: set_error_log_locations
 	$(prove_check)
 
 installcheck:
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 3d85857bfa..5fbcb5e34d 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -13,6 +13,8 @@
 PGFILEDESC = "pg_regress - test driver"
 PGAPPICON = win32
 
+override ERROR_LOG_LOCATIONS = $(abs_top_builddir)/$(subdir)/log
+
 subdir = src/test/regress
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global
@@ -128,10 +130,10 @@ tablespace-setup:
 
 REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 $(EXTRA_REGRESS_OPTS)
 
-check: all tablespace-setup
+check: all set_error_log_locations tablespace-setup
 	$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
 
-check-tests: all tablespace-setup | temp-install
+check-tests: all set_error_log_locations tablespace-setup | temp-install
 	$(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS)
 
 installcheck: all tablespace-setup

Re: Finding cause of test fails on the cfbot site

[Andrew Dunstan]

On 2/21/21 9:28 PM, Thomas Munro wrote:
> On Sat, Feb 20, 2021 at 3:54 AM Andrew Dunstan  wrote:
>> here's a very small and simple (and possibly naive)  POC patch that
>> demonstrates this and seems to do the right thing.
> As a small variation that might be more parallelism-friendly, would it
> be better to touch a file with a known name in any subdirectory that
> contains potentially interesting logs, and rm it when a test succeeds?



Yes, that sounds better, Thanks.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Finding cause of test fails on the cfbot site

[Andrew Dunstan]

On 2/21/21 10:34 PM, Andres Freund wrote:
> Hi,
>
> On 2021-02-17 15:18:02 -0500, Andrew Dunstan wrote:
>> yeah. The cfbot runs check-world which makes it difficult for it to know
>> which log files to show when there's an error. That's a major part of
>> the reason the buildfarm runs a much finer grained set of steps.
> I really think we need a better solution for this across the different
> use-cases of running tests. For development parallel check-world is
> important for a decent hack-test loop. But I waste a fair bit of time to
> scroll back to find the original source of failures. And on the
> buildfarm we waste a significant amount of time by limiting parallelism
> due to the non-parallel sequence of finer grained steps.


Maybe but running fast isn't really a design goal of the buildfarm. It's
meant to be automated so it doesn't matter if it takes 10 or 20 or 60
minutes.


Another reason for using fine grained tasks is to be able to
include/exclude them as needed. See the buildfarm's
exclude-steps/only-steps parameters.


That said there is some provision for parallelism, in that multiple
branches and multiple members can be tested at the same time, and
run_branches.pl will manage that fairly nicely for you. See
<https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto#Running_in_Parallel>
for details


> And it's not just about logs - even just easily seeing the first
> reported test failure without needing to search through large amounts of
> text would be great.
>
> With, um, more modern buildtools (e.g. ninja) you'll at least get the
> last failure displayed at the end, instead of seing a lot of other
> things after it like with make.
>
>
> My suspicion is that, given the need to have this work for both msvc and
> make, writing an in-core test-runner script is the only real option to
> improve upon the current situation.



Ok ... be prepared for a non-trivial maintenance cost, however, which
will be born by those of us fluent in perl, the only realistic
possibility unless we want to add to build dependencies. That's far from
everyone.


Part of the problem that this isn't going to solve is the sheer volume
that some tests produce. For example, the pg_dump tests produce about
40k lines  / 5Mb of log.



> For make it'd not be hard to add a recursive 'listchecks' target listing
> the individual tests that need to be run. Hacking up vcregress.pl to do
> that, instead of what it currently does, shouldn't be too hard either.
>
>
> Once there's a list of commands that need to be run it's not hard to
> write a loop in perl that runs up to N tests in parallel, saving their
> output. Which then allows to display the failing test reports at the
> end.
>
>
> If we then also add a convention that each test outputs something like
> TESTLOG: path/to/logfile
> ...
> it'd not be hard to add support for the test runner to list the files
> that cfbot et al should output.


Yeah, there is code in the buildfarm that contains a lot of building
blocks that can be used for this sort of stuff, see the PGBuild::Utils
and PGBuild::Log modules.


> Looking around the tree, the most annoying bit to implement something
> like this is that things below src/bin/, src/interfaces, src/test,
> src/pl implement their own check, installcheck targets. Given the number
> of these that just boil down to a variant of
>
> check:
>   $(pg_regress_check)
> $(prove_check)
> installcheck:
>   $(pg_regress_installcheck)
>
> it seems we should lift the REGRESS and TAP_TESTS specific logic in
> pgxs.mk up into src/Makefiles.global. Which then would make something
> list a global listchecks target easy.
>

Yeah, some of this stuff has grown a bit haphazardly, and maybe needs
some rework.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Allow matching whole DN from a client certificate

[Andrew Dunstan]

On 2/26/21 2:55 PM, Jacob Champion wrote:
> On Sat, 2021-01-30 at 16:18 -0500, Andrew Dunstan wrote:
>> Making incremental additions to the certificate set easier wouldn't be a
>> bad thing.
>>
>> I wonder if we should really be setting 1 as the serial number, though.
>> Might it not be better to use, say, `date +%Y%m%d01` rather like we do
>> with catalog version numbers?
> I have been experimenting a bit with both of these suggestions; hope to
> have something in time for commitfest on Monday. Writing new tests for
> NSS has run into the same problems you've mentioned.
>
> FYI, I've pulled the port->peer_dn functionality you've presented here
> into my authenticated identity patchset at [1].
>
> --Jacob
>
> [1] 
> https://www.postgresql.org/message-id/flat/c55788dd1773c521c862e8e0dddb367df51222be.camel%40vmware.com


Cool.


I think the thing that's principally outstanding w.r.t. this patch is
what format we should use to extract the DN. Should we use RFC2253,
which reverses the field order, as has been suggested upthread and is in
the latest patch? I'm slightly worried that it might be a POLA
violation. But I don't have terribly strong feelings about it.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Why does the BF sometimes not dump regression.diffs?

[Andrew Dunstan]

On 3/1/21 4:05 PM, Thomas Munro wrote:
> Hello,
>
> I saw this failure after a recent commit (though the next build
> succeeded, and I don't yet have any particular reason to believe that
> the commits it blamed are at fault, we'll see...):
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gombessa&dt=2021-03-01%2004%3A58%3A17
>
> Strangely, it didn't spit out the regression.diffs so I don't know
> anything more than "commit_ts failed".  I emailed the owner asking for
> that log but I see now it's too late.  I wonder if there is some
> scripting in there that doesn't work correctly on OpenBSD for whatever
> reason...  I know that we *sometimes* get regression.diffs from
> failures in here, but I haven't tried to figure out the pattern (which
> animals etc).  Here's a similar failure that does show the .diffs I
> wish I could see, same BF client version (11), on Windows:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2020-07-13%2000:47:47
>
>


The version numbering is a bit misleading on fairywren, which, as it's a
machine I control runs from a git checkout, which clearly is later than
REL_11 even though that's what the version string says. Commit 13d2143
fixed this. It was included in Release 12.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: buildfarm windows checks / tap tests on windows

[Andrew Dunstan]

On 3/1/21 3:07 PM, Andres Freund wrote:
> Hi,
>
> As part of trying to make the aio branch tests on cirrus CI pass with
> some tap tests I noticed that "src/tools/msvc/vcregress.pl recoverycheck"
> hangs. A long phase of remote debugging later I figured out that that's
> not a fault of the aio branch - it also doesn't pass on master (fixed in
> [1]).
>
> Which confused me - shouldn't the buildfarm have noticed? But it seems
> that all msvc animals either don't have tap tests enabled or they
> disable 'misc-checks' which in turn is what the buildfarm client uses to
> trigger all the 'recovery' checks.
>
> It seems we're not just skipping recovery, but also e.g. tap tests in
> contrib, all the tests in src/test/modules/...
>
> Andrew, what's the reason for that? Is it just that they hung at some
> point? Were too slow?



I don't think speed is the issue. I probably disabled misc-tests on
drongo and bowerbird (my two animals in question) because I got  either
instability or errors I was unable to diagnose. I'll go back and take
another look to narrow this down. It's possible to disable individual tests.



>
>
> On that last point: Running the tap tests on windows appears to be
> *excruciatingly* slow. How does anybody develop on windows without a
> mechanism to actually run tests in parallel?


I think most people develop elsewhere and then adapt/test on Windows if
necessary.


>
> I think it'd be good if vcregress.pl at least respected PROVE_FLAGS from
> the environment - it can't currently be passed in for several of the
> vcregress.pl tests, and it does seem to make things to be at least
> somewhat less painful.



+1


>
>
> This makes it even clearer to me that we really need a builtin
> testrunner that runs tests efficiently *and* debuggable on windows.
>

"show me the code" :-)


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com