Re: [HACKERS] Potential GIN vacuum bug

[Heikki Linnakangas]

On 08/16/2015 12:58 AM, Jeff Janes wrote:

When ginbulkdelete gets called for the first time in a  VACUUM(i.e. stats
== NULL), one of the first things it does is call ginInsertCleanup to get
rid of the pending list.  It does this in lieu of vacuuming the pending
list.

This is important because if there are any dead tids still in the Pending
list, someone else could come along during the vacuum and post the dead
tids into a part of the index that VACUUM has already passed over.

The potential bug is that ginInsertCleanup exits early (ginfast.c lines
796, 860, 898) if it detects that someone else is cleaning up the pending
list, without waiting for that someone else to finish the job.

Isn't this a problem?


Yep, I think you're right. When that code runs as part of VACUUM, it 
should not give up like that.


Hmm, I see other race conditions in that code too. Even if VACUUM wins 
the race you spotted, and performs all the insertions, reaches the end 
of the pending items list, and deletes the pending list pages, it's 
possible that another backend started earlier, and is still processing 
the same items from the pending items list. It will add them to the 
tree, and after it's finished with that it will see that the pending 
list page was already deleted, and bail out. But if there is a dead 
tuple in the pending items list, you have trouble. The other backend 
will re-insert it, and that might happen after VACUUM had already 
removed it from the tree.


Also, ginInsertCleanup() seems to assume that if another backend has 
just finished cleaning up the same page, it will see the page marked as 
deleted. But what if the page is not only marked as deleted, but also 
reused for something else already?


- Heikki



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

Re: [HACKERS] Error message with plpgsql CONTINUE

[Pavel Stehule]

2015-08-17 6:19 GMT+02:00 Jim Nasby :

> Calling CONTINUE with a label that's not a loop produces an error message
> with no context info [1]. This is because of
>
> rc = exec_stmt_block(&estate, func->action);
> if (rc != PLPGSQL_RC_RETURN)
> {
> estate.err_stmt = NULL;
> estate.err_text = NULL;
>
> I trawled through git blame a bit and it looks like it's been that way for
> a very long time.
>
> I think err_stmt should probably only be reset in the non-return case a
> bit below that. I'm not sure about err_text though. Also, the code treats
> PLPGSQL_RC_OK and PLPGSQL_RC_EXIT the same, which seems like a bug; I would
> think PLPGSQL_RC_EXIT should be handled the same way as CONTINUE.
>
> If someone can confirm this and tell me what to do about err_text I'll
> submit a patch.
>

maybe "during function exit" ?

Regards

Pavel


>
> [1]
> decibel@decina.attlocal/50703=# do $$
> begin
> <>
> for a in 1..3 loop
> <>
> BEGIN
> <>
> for b in 8..9 loop
> if a=2 then
> continue sub;
> end if;
> raise notice '% %', a, b;
> end loop inner;
> END sub;
> end loop outer;
> end;
> $$;
> NOTICE:  1 8
> NOTICE:  1 9
> ERROR:  CONTINUE cannot be used outside a loop
> CONTEXT:  PL/pgSQL function inline_code_block
> decibel@decina.attlocal/50703=#
>
> [2]
> https://github.com/postgres/postgres/blob/83604cc42353b6c0de2a3f3ac31f94759a9326ae/src/pl/plpgsql/src/pl_exec.c#L438
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] Configure checks and optimizations/crc32 tests

[Heikki Linnakangas]

On 08/14/2015 07:42 PM, Andres Freund wrote:

Going over my vpaths I noticed another problem with the test. With gcc I
get slice-by-8 instead of the runtime variant:

checking for builtin __atomic int64 atomic operations... yes
checking for __get_cpuid... yes
checking for __cpuid... no
checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=... no
checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=-msse4.2... no
checking which CRC-32C implementation to use... slicing-by-8

That's because I get a warning
conftest.c:179:1: warning: old-style function definition 
[-Wold-style-definition]
  main ()
  ^
and PGAC_SSE42_CRC32_INTRINSICS turns on ac_c_werror_flag. Now I can
work around this by , but I don't really see why that test needs to turn on
-Werror? Isn't the point of using a linker test that we'd get a proper
linker failure if the functions don't exist?


Yeah, it was probably just copy-pasted from the other macros in 
c-compiler.m4 without thinking.


- Heikki



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

[HACKERS] Memory allocation in spi_printtup()

[Neil Conway]

spi_printtup() has the following code (spi.c:1798):

if (tuptable->free == 0)
{
tuptable->free = 256;
tuptable->alloced += tuptable->free;
tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,

   tuptable->alloced * sizeof(HeapTuple));
}

i.e., it grows the size of the tuptable by a fixed amount when it runs
out of space. That seems odd; doubling the size of the table would be
more standard. Does anyone know if there is a rationale for this
behavior?

Attached is a one-liner to double the size of the table when space is
exhausted. Constructing a simple test case in which a large result is
materialized via SPI_execute() (e.g., by passing two large queries to
crosstab() from contrib/tablefunc), this change reduces the time
required to exceed the palloc size limit from ~300 seconds to ~93
seconds on my laptop.

Of course, using SPI_execute() rather than cursors for queries with
large result sets is not a great idea, but there is demonstrably code
that does this (e.g., contrib/tablefunc -- I'll send a patch for that
shortly).

Neil
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index d544ad9..2573b3f 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1797,7 +1797,7 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *self)
 
 	if (tuptable->free == 0)
 	{
-		tuptable->free = 256;
+		tuptable->free = tuptable->alloced;
 		tuptable->alloced += tuptable->free;
 		tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,
 	  tuptable->alloced * sizeof(HeapTuple));

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

[HACKERS] Error message with plpgsql CONTINUE

[Jim Nasby]

Calling CONTINUE with a label that's not a loop produces an error 
message with no context info [1]. This is because of


rc = exec_stmt_block(&estate, func->action);
if (rc != PLPGSQL_RC_RETURN)
{
estate.err_stmt = NULL;
estate.err_text = NULL;

I trawled through git blame a bit and it looks like it's been that way 
for a very long time.


I think err_stmt should probably only be reset in the non-return case a 
bit below that. I'm not sure about err_text though. Also, the code 
treats PLPGSQL_RC_OK and PLPGSQL_RC_EXIT the same, which seems like a 
bug; I would think PLPGSQL_RC_EXIT should be handled the same way as 
CONTINUE.


If someone can confirm this and tell me what to do about err_text I'll 
submit a patch.


[1]
decibel@decina.attlocal/50703=# do $$
begin
<>
for a in 1..3 loop
<>
BEGIN
<>
for b in 8..9 loop
if a=2 then
continue sub;
end if;
raise notice '% %', a, b;
end loop inner;
END sub;
end loop outer;
end;
$$;
NOTICE:  1 8
NOTICE:  1 9
ERROR:  CONTINUE cannot be used outside a loop
CONTEXT:  PL/pgSQL function inline_code_block
decibel@decina.attlocal/50703=#

[2] 
https://github.com/postgres/postgres/blob/83604cc42353b6c0de2a3f3ac31f94759a9326ae/src/pl/plpgsql/src/pl_exec.c#L438

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] TAP tests are badly named

[Michael Paquier]

On Mon, Aug 17, 2015 at 7:15 AM, Noah Misch  wrote:
> On Sun, Aug 16, 2015 at 05:08:56PM -0400, Andrew Dunstan wrote:
>> On 08/16/2015 02:23 PM, Noah Misch wrote:
>> >>-sub tapcheck
>> >>+sub tap_check
>> >>  {
>> >>-   InstallTemp();
>> >>+   die "Tap tests not enabled in configuration"
>> >>+ unless $config->{tap_tests};
>> >I endorse Heikki's argument for having omitted the configuration flag:
>> >http://www.postgresql.org/message-id/55b90161.5090...@iki.fi
>>
>>
>> That argument is not correct.  None of the tap tests can be run via make if
>> --enable-tap-tests is not set. This doesn't just apply to the check-world
>> target as Heikki asserted. Have a look at the definitions of prove_check and
>> prove_installcheck in src/Makefile.global.in if you don't believe me.
>
> While that one piece of Heikki's argument was in error, I didn't feel that it
> affected the conclusion.  Please reply to the aforementioned email to dispute
> the decision; some involved parties may not be following this thread.

FWIW, I agree with Andrew's approach here. We are going to need this
parameter switch once TAP routines are used in another code path than
src/bin to control if a test can be run or not based on the presence
of t/ and the value of this parameter. See for example the patch aimed
at testing dumpable tables with pg_dump which should be part of the
target modulescheck as argued upthread.
My 2c.
-- 
Michael


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

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Sun, Aug 16, 2015 at 05:08:56PM -0400, Andrew Dunstan wrote:
> On 08/16/2015 02:23 PM, Noah Misch wrote:
> >>-sub tapcheck
> >>+sub tap_check
> >>  {
> >>-   InstallTemp();
> >>+   die "Tap tests not enabled in configuration"
> >>+ unless $config->{tap_tests};
> >I endorse Heikki's argument for having omitted the configuration flag:
> >http://www.postgresql.org/message-id/55b90161.5090...@iki.fi
> 
> 
> That argument is not correct.  None of the tap tests can be run via make if
> --enable-tap-tests is not set. This doesn't just apply to the check-world
> target as Heikki asserted. Have a look at the definitions of prove_check and
> prove_installcheck in src/Makefile.global.in if you don't believe me.

While that one piece of Heikki's argument was in error, I didn't feel that it
affected the conclusion.  Please reply to the aforementioned email to dispute
the decision; some involved parties may not be following this thread.

> >>+   # Reset those values, they may have been changed by another test.
> >>+   # XXX is this true?
> >>+   local %ENV = %ENV;
> >>$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
> >>$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
> >>+   $ENV{TESTDIR} = "$dir";
> >The comment pertained only to the TESTDIR environment variable.  Adding the
> >local %ENV is unnecessary.  I think you can just delete the comment; there's
> >nothing noteworthy about setting a different TESTDIR value for each suite.
> >The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, 
> >though
> >keeping them here seems good for the benefit of future TAP targets.
> 
> The local decl is clearly needed. Otherwise repeated invocations of the
> function will result in repeated prepending of $topdir/src/test/perl to
> PERL5LIB. It's incredibly cheap anyway. And as you say, this is a much
> better place to do that than in bincheck. If you prefer, I could dispense
> with the local and instead only set to PERL5LIB conditionally. It's just a
> matter of style.

With the comment gone, the way you've done this section is fine.


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

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/16/2015 02:23 PM, Noah Misch wrote:

-sub tapcheck
+sub tap_check
  {
-   InstallTemp();
+   die "Tap tests not enabled in configuration"
+ unless $config->{tap_tests};

I endorse Heikki's argument for having omitted the configuration flag:
http://www.postgresql.org/message-id/55b90161.5090...@iki.fi



That argument is not correct.  None of the tap tests can be run via make 
if --enable-tap-tests is not set. This doesn't just apply to the 
check-world target as Heikki asserted. Have a look at the definitions of 
prove_check and prove_installcheck in src/Makefile.global.in if you 
don't believe me.





+   # Reset those values, they may have been changed by another test.
+   # XXX is this true?
+   local %ENV = %ENV;
$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
  
+	$ENV{TESTDIR} = "$dir";

The comment pertained only to the TESTDIR environment variable.  Adding the
local %ENV is unnecessary.  I think you can just delete the comment; there's
nothing noteworthy about setting a different TESTDIR value for each suite.
The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, though
keeping them here seems good for the benefit of future TAP targets.




The local decl is clearly needed. Otherwise repeated invocations of the 
function will result in repeated prepending of $topdir/src/test/perl to 
PERL5LIB. It's incredibly cheap anyway. And as you say, this is a much 
better place to do that than in bincheck. If you prefer, I could 
dispense with the local and instead only set to PERL5LIB conditionally. 
It's just a matter of style.


cheers

andrew


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

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Fri, Aug 14, 2015 at 09:31:43AM -0400, Andrew Dunstan wrote:
> On 08/14/2015 09:27 AM, Andrew Dunstan wrote:
> >The code
> >is rearranged a little, and an incorrect piece of code setting
> >$ENV{PERL5LIB} is fixed (in the case where it was previously empty it
> >would have added a spurious ";" and possibly caused a warning as well).

The PERL5LIB bit is clearly good, but please commit it separately.

> >Instead of looking everywhere in the tree for /t directories, the new
> >bincheck function only looks for them in src/bin.

Check.

> >And the tests would die
> >on the first failure, as we would also expect the checks run under "make"
> >to do.

"make" offers a choice.  If I had to settle for just one of its behaviors, I
would choose -k.  One can emulate "make -S" by killing "make -k" after the
first error, but there's no straightforward way to emulate "make -k" in terms
of "make -S".  Thus, I prefer the ancient vcregress decision in this area.  In
any event, a proposal to change it would need its own thread and a patch that
changes all targets facing this decision.

> >The effect is to remove the target "tapcheck" for which there is no "make"
> >equivalent, and replace it with the target "bincheck", which is the
> >equivalent of "make -C src/bin installcheck", which happens to be what the
> >buildfarm runs.

This "vcregress bincheck" differs from "make -C src/bin installcheck" by using
"check" semantics, and it differs from "make -C src/bin check" by skipping the
pg_upgrade test suite.  (I don't have any point in saying that beyond
clarifying the record.)

> -sub tapcheck
> +sub tap_check
>  {
> - InstallTemp();
> + die "Tap tests not enabled in configuration"
> +   unless $config->{tap_tests};

I endorse Heikki's argument for having omitted the configuration flag:
http://www.postgresql.org/message-id/55b90161.5090...@iki.fi

> + # Reset those values, they may have been changed by another test.
> + # XXX is this true?
> + local %ENV = %ENV;
>   $ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
>   $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
>  
> + $ENV{TESTDIR} = "$dir";

The comment pertained only to the TESTDIR environment variable.  Adding the
local %ENV is unnecessary.  I think you can just delete the comment; there's
nothing noteworthy about setting a different TESTDIR value for each suite.
The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, though
keeping them here seems good for the benefit of future TAP targets.


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

Re: [HACKERS] Raising our compiler requirements for 9.6

[Tom Lane]

Andres Freund  writes:
> On 2015-08-15 23:50:09 -0400, Noah Misch wrote:
>> Solaris Studio 12.3 (newest version as of Oct 2014) still does that
>> when optimization is disabled, and I place sufficient value on keeping
>> inlining enabled for such a new compiler.

> Ah, that's cool. I was wondering generally how we could find an animal
> to detect that case once pademelon met its untimely (or timely by now?)
> end.

Yeah.  If we get to the point where we can't actually find any toolchains
that work that way, it may be time to revise our portability policy.  But
for now Solaris Studio is a good-enough reason to not move the goalposts.

>> The policy would then be
>> (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a
>> backend symbol.  When a header is dedicated to such functions, we might avoid
>> the whole header in the frontend instead of wrapping each function.  That
>> policy works for me.

Works for me as well, as long as we have buildfarm critters that will
notice oversights.

regards, tom lane


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

Re: [HACKERS] Test code is worth the space

[Greg Stark]

On Sun, Aug 16, 2015 at 7:33 AM, Noah Misch  wrote:
> When I've just spent awhile implementing a behavior change, the test diffs are
> a comforting sight.  They confirm that the test suite exercises the topic I
> just changed.  Furthermore, most tests today do not qualify under this
> stringent metric you suggest.  The nature of pg_regress opposes it.

It's not a comforting sight for me. It means I need to change the test
results and then of course the tests will pass. So they didn't really
test anything and I don't really know whether the changes broke
anything. Any test I had to adjust for my change was effectively
worthless.

This is precisely my point about pg_regress and why it's the reason
there's pressure not to have extensive tests. It's useful to have some
end-to-end black box tests like this but it's self-limiting. You can't
test many details without tying the tests to specific behaviour.

I have worked with insanely extensive testing suites that didn't have
this problem. But they were unit tests for code that was structured
around testability. When the API is designed to be testable and you
have good infrastructure for mocking up the environment and comparing
function results in a much narrower way than just looking at text
output you can test the correctness of each function without tying the
test to the specific behaviour.

*That* gives a real comfort. When you change a function to behave
entirely differently and can still run all the tests and see that all
the code that depends on it still operate correctly then you know you
haven't broken anything. In that case it actually *speeds up*
development rather than slowing it down. It lets newbie developers
hack on code where they don't necessarily know all the downstream
dependent code and not be paralyzed by fear that they'll break
something.

-- 
greg


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

Re: [HACKERS] Management of simple_eval_estate for plpgsql DO blocks

[Simon Riggs]

On 14 August 2015 at 17:42, Tom Lane  wrote:


> The simplest fix for this would be to give up on the idea that DO blocks
> use private simple_eval_estates, and make them use the shared one.
> However, that would result in intra-transaction memory bloat for
> transactions executing large numbers of DO blocks; see commit c7b849a89,
> which installed that arrangement to begin with.  Since that change was
> based on a user complaint, this answer doesn't seem appetizing.
>

...


> Or we could change things so that DO blocks use private cast_hash
> hashtables along with their private simple_eval_estates.  This would
> give up some efficiency (since a DO block would then always need to do
> its own cast lookups) but it would be a simple and reliable fix.
>
> I'm kind of inclined to go with the last choice, but I wonder if anyone
> wants to argue differently, or sees another feasible solution.
>

Not everyone uses large numbers of DO blocks, but casts apply everywhere,
right?

If the choice is efficiency or memory bloat, can't we choose a point where
we switch from one to the other? i.e. use private structures after N uses
of the shared structures.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Raising our compiler requirements for 9.6

[Noah Misch]

On Fri, Aug 14, 2015 at 08:40:13PM +0200, Andres Freund wrote:
> > > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote:
> > > > > + * lockdefs.h
> > > > > + *  Frontend exposed parts of postgres' low level lock mechanism

> I actually think the split came out to work far better than I'd
> anticipated. Having a slimmed-down version of lock.h for code that just
> needs to declare/pass lockmode parameters seems to improve our layering
> considerably.  I got round to Alvaro's and Roberts position that
> removing lock.h from namespace.h and heapam.h would be a really nice
> improvemen on that front.

I assessed 0001-WIP-Decrease-usage-of-lock.h-further.patch and reassessed
0001-Don-t-include-low-level-locking-code-from-frontend-c.patch (commit
4eda0a6).  I changed the details of my position compared to my last review.

As we see from the patches' need to add #include statements to .c files and
from Robert's report of a broken EDB build, this change creates work for
maintainers of non-core code, both backend code (modules) and frontend code
(undocumented, but folks do it).  That's to be expected and doesn't take a
great deal of justification, but users should get benefits in connection with
the work.  This brings to mind the htup_details.h introduction, which created
so much busy work in non-core code.  I don't expect the lock.h split to be
quite that disruptive.  Statistics on PGXN modules:

29 modules mention htup_details.h
 8 modules mention lwlock.h
 7 modules mention LWLock
 4 modules mention lock.h
 1 module mentions LockAcquire()

Four modules (imcs, pg_stat_kcache, query_histogram, query_recorder) mentioned
LWLock without mentioning lwlock.h.  These patches (trivially) break the imcs
build.  The other three failed to build for unrelated reasons, but I suspect
these patches give those modules one more thing to update.  What do users get
out of this?  They'll learn if their code is not portable to pademelon, but
#warning "atomics.h in frontend code is not portable" would communicate the
same without compelling non-core code to care.  Other than that benefit,
making headers #error-on-FRONTEND mostly lets us congratulate ourselves for
having introduced the start of a header layer distinction.  I'd be for that if
PostgreSQL were new, but I can't justify doing it at the user cost already
apparent.  That would be bad business.

Therefore, I urge you to instead add the aforementioned #warning and wrap with
#ifdef FRONTEND the two #include "port/atomics.h" in headers.  That tightly
limits build breakage; it can only break frontend code, which is rare outside
core.  Hackers will surely notice if a patch makes the warning bleat, so
mistakes won't survive long.  Non-core frontend code, if willing to cede a
shred of portability, can experiment with atomics.  Folks could do nifty
things with that.

Thanks,
nm


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