Re: [HACKERS] Compile warnings on OSX 10.10 clang 6.0

[Michael Paquier]

On Sat, Apr 4, 2015 at 6:21 AM, Tom Lane  wrote:
> I wrote:
>> Peter Eisentraut  writes:
>>> These warnings also happen with older versions of clang.  Now idea how
>>> to fix yet.  I'm thinking that clang should be fixed, because these
>>> warnings are stupid.
>
>> Yeah, they're utterly stupid; whoever put them in obviously doesn't
>> have a clue about typical Makefile construction.  I wonder if next
>> we'll see complaints about unnecessary -D or -I switches.
>
>> Having said that, I did look awhile ago about how we might get rid of
>> them, and it seems not easy; for starters we would need to drop the
>> assumption that CFLAGS can always be included when linking.  Also,
>> AFAICT -pthread sometimes *is* required when linking; so it's
>> not even very obvious when to suppress the switch, even if we could
>> do so without wholesale rearrangement of our FLAGS handling.
>
> On the other hand, there's often more than one way to skin a cat.
> It occurred to me that maybe we could just turn off this class of warning,
> and after some experimentation I found out that
> "-Wno-unused-command-line-argument" does that, at least in the version
> of clang that Apple's currently shipping.
>
> Who's for enabling that if the compiler takes it?

Yes, please. I always found those pthread warnings annoying.
-- 
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] Abbreviated keys for Numeric

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 >> For those following along at home, the failures are on these queries:

 >> SELECT 1.1 AS two UNION SELECT 2.2;
 >> SELECT 1.1 AS two UNION SELECT 2;
 >> SELECT 1 AS two UNION SELECT 2.2;
 >> SELECT 1.1 AS three UNION SELECT 2 UNION ALL SELECT 2;

 >> In each case, the expected result is with the values in ascending
 >> numerical order.  In each case, the 1 or 1.1 value which ought to
 >> appear before 2 or 2.2 instead appears after it.  Strictly speaking,
 >> this is not the wrong answer to the query, and could be perhaps
 >> explained by the planner choosing a hash aggregate rather than a sort
 >> + unique plan.  But this patch doesn't change the planner at all, so
 >> the plan should be the same as it has always been.

 Tom> Yeah.  We could add an EXPLAIN to make certain, perhaps, but since
 Tom> none of the other critters are failing I doubt this explanation.

This failure in the union.sql test is exactly the one that happens if
you build with --disable-float8-byval, for what that's worth.

Does the windows build perhaps not define USE_FLOAT8_BYVAL? that would
explain the breakage.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Providing catalog view to pg_hba.conf file - Patch submission

[Pavel Stehule]

Hi

2015-03-31 14:38 GMT+02:00 Haribabu Kommi :

> On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule 
> wrote:
> > Hi
> >
> > I checked this patch. I like the functionality and behave.
>
> Thanks for the review.
>
> Here I attached updated patch with the following changes.
>
> 1. Addition of two new keyword columns


> keyword_databases - The database name can be "all", "replication",
> sameuser", "samerole" and "samegroup".
> keyword_roles - The role can be "all" and a group name prefixed with "+".
>

I am not very happy about name - but I have no better idea :( - maybe
"database_mask", "user_mask"


>
> The rest of the database and role names are treated as normal database
> and role names.
>
> 2. Added the code changes to identify the names with quoted.
>

Is it a good idea? Database's names are not quoted every else (in system
catalog). So now, we cannot to use join to this view.

postgres=# select (databases)[1] from pg_hba_conf ;
 databases
---
 "omega 2"

(4 rows)

postgres=# select datname from pg_database ;
  datname
---
 template1
 template0
 postgres
 omega 2
(4 rows)

I dislike this - we know, so the name must be quoted in file (without it,
the file was incorrect). And if you need quotes, there is a function
quote_ident. If we use quotes elsewhere, then it should be ok, bot not now.
Please, remove it. More, it is not necessary, when you use a "keyword"
columns.

Regards

Pavel






>
> 3. Updated documentation changes
>
> 4. Regression test is corrected.
>
>
> Regards,
> Hari Babu
> Fujitsu Australia
>

Re: [HACKERS] Abbreviated keys for Numeric

[Tom Lane]

Robert Haas  writes:
> For those following along at home, the failures are on these queries:

> SELECT 1.1 AS two UNION SELECT 2.2;
> SELECT 1.1 AS two UNION SELECT 2;
> SELECT 1 AS two UNION SELECT 2.2;
> SELECT 1.1 AS three UNION SELECT 2 UNION ALL SELECT 2;

> In each case, the expected result is with the values in ascending
> numerical order.  In each case, the 1 or 1.1 value which ought to
> appear before 2 or 2.2 instead appears after it.  Strictly speaking,
> this is not the wrong answer to the query, and could be perhaps
> explained by the planner choosing a hash aggregate rather than a sort
> + unique plan.  But this patch doesn't change the planner at all, so
> the plan should be the same as it has always been.

Yeah.  We could add an EXPLAIN to make certain, perhaps, but since
none of the other critters are failing I doubt this explanation.

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] Abbreviated keys for Numeric

[Robert Haas]

On Fri, Apr 3, 2015 at 3:46 PM, Andrew Gierth
 wrote:
> (This does rather suggest to me that some better regression tests for
> sorting would be a good idea, possibly even including on-disk sorts.)

Yeah.  I've been unpleasantly surprised by how easy it is to pass the
regression tests with sorting broken.

>  >> If you're determined to go this route - over my protest - then you
>  >> need to do something like define a NumericAbbrevGetDatum(x) macro
>  >> and use it in place of the Int64GetDatum / Int32GetDatum ones for
>  >> both NAN and the return from numeric_abbrev_convert_var.
>
>  Robert> Patch for that attached.
>
> That looks reasonable, though I think it could do with a comment
> explaining _why_ it's defining its own macros rather than using
> Int32*/Int64*. (And I wrote that before seeing Tom's message, even.)

Agreed.  I have added that and committed this.

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


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

Re: [HACKERS] Abbreviated keys for Numeric

[Robert Haas]

On Fri, Apr 3, 2015 at 5:45 PM, Tom Lane  wrote:
> ... btw, has anyone noticed that this patch broke hamerkop and
> bowerbird?  Or at least, it's hard to see what other recent commit
> would explain the failures they're showing.

I noticed the failure on bowerbird today and I agree that looks an
awful lot like it might be this patch's fault.  The failure on
hamerkop looks like the same issue.  I'm a bit at a loss to explain
exactly what has gone wrong there.  What those machines have in common
is that they are the only 64-bit Windows machines using the Microsoft
toolchain in the buildfarm, but I don't know why that should provoke a
failure.  I seem to remember having some trouble previously with
Microsoft compilers being finickier than others about a shift with a
width greater than or equal to the bit-width of the value, but if
there is such a problem here, I can't spot it.  Nor do I see a
compiler warning in numeric.c which might provide a clue.

For those following along at home, the failures are on these queries:

SELECT 1.1 AS two UNION SELECT 2.2;
SELECT 1.1 AS two UNION SELECT 2;
SELECT 1 AS two UNION SELECT 2.2;
SELECT 1.1 AS three UNION SELECT 2 UNION ALL SELECT 2;

In each case, the expected result is with the values in ascending
numerical order.  In each case, the 1 or 1.1 value which ought to
appear before 2 or 2.2 instead appears after it.  Strictly speaking,
this is not the wrong answer to the query, and could be perhaps
explained by the planner choosing a hash aggregate rather than a sort
+ unique plan.  But this patch doesn't change the planner at all, so
the plan should be the same as it has always been.  And if it is
choosing to sort, then it's clearly doing it wrong, but there doesn't
seem to be anything platform-specific about this code, so I'm
mystified.

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


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

[HACKERS] COALESCE() query yield different result with MJ vs. NLJ/HJ

[Qingqing Zhou]

The symptom is that the same join query yield different results with
MJ and NLJ/HJ.  Here is a repro:

---
create table t1(a int);create table t2(b int);
insert into t1 values(10); insert into t2 values(2);
analyze t1; analyze t2;
set enable_mergejoin=on; set enable_nestloop=off; set enable_hashjoin=off;
explain analyze select a, b from t1 left join  t2 on coalesce(a, 1) =
coalesce(b,1)  where (coalesce(b,1))>0
set enable_mergejoin=off; set enable_nestloop=on; set enable_hashjoin=off;
explain analyze select a, b from t1 left join  t2 on coalesce(a, 1) =
coalesce(b,1)  where (coalesce(b,1))>0
---

A possible explanation is that in fix_join_expr_mutator(), we optimize
with the case that if child node already compute an expression then
upper node shall reuse it. In MJ, as coalesce() already computed in
sort node, thus the NULL is directly used for ExecQual(>0) for join
filter.

If we take out this optimization the problem is solved but may looks
like an overkill. What's a better fix?

Thanks,
Qingqing


-- 
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] Sloppy SSPI error reporting code

[Noah Misch]

On Thu, Apr 02, 2015 at 07:31:52AM -0400, Bruce Momjian wrote:
> On Thu, Apr  2, 2015 at 01:44:59AM -0400, Noah Misch wrote:
> > On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
> > > On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
> > > > While looking at fe-auth.c I noticed quite a few places that weren't
> > > > bothering to make error messages localizable (ie, missing libpq_gettext
> > > > calls), and/or were failing to add a trailing newline as expected in
> > > > libpq error messages.  Perhaps these are intentional but I doubt it.
> > > > Most of the instances seemed to be SSPI-related.
> > > > 
> > > > I have no intention of fixing these myself, but whoever committed that
> > > > code should take a second look.
> > > 
> > > I looked through that file and only found two cases;  patch attached.
> > 
> > Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
> 
> Oh, I accidentally saw the backend version of that function, which
> looked fine.  I have attached a patch for that.

That patch looks reasonable.


-- 
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] Compile warnings on OSX 10.10 clang 6.0

[John Gorman]

I have confirmed that "-Wno-unused-command-line-argument"
suppresses the "-pthread" warning for clang 6.0 and does not
trigger a warning in gcc 4.9.

Works for me!

John

On Fri, Apr 3, 2015 at 5:21 PM, Tom Lane  wrote:

> I wrote:
> > Peter Eisentraut  writes:
> >> These warnings also happen with older versions of clang.  Now idea how
> >> to fix yet.  I'm thinking that clang should be fixed, because these
> >> warnings are stupid.
>
> > Yeah, they're utterly stupid; whoever put them in obviously doesn't
> > have a clue about typical Makefile construction.  I wonder if next
> > we'll see complaints about unnecessary -D or -I switches.
>
> > Having said that, I did look awhile ago about how we might get rid of
> > them, and it seems not easy; for starters we would need to drop the
> > assumption that CFLAGS can always be included when linking.  Also,
> > AFAICT -pthread sometimes *is* required when linking; so it's
> > not even very obvious when to suppress the switch, even if we could
> > do so without wholesale rearrangement of our FLAGS handling.
>
> On the other hand, there's often more than one way to skin a cat.
> It occurred to me that maybe we could just turn off this class of warning,
> and after some experimentation I found out that
> "-Wno-unused-command-line-argument" does that, at least in the version
> of clang that Apple's currently shipping.
>
> Who's for enabling that if the compiler takes it?
>
> regards, tom lane
>

Re: [HACKERS] Abbreviated keys for Numeric

[Tom Lane]

... btw, has anyone noticed that this patch broke hamerkop and
bowerbird?  Or at least, it's hard to see what other recent commit
would explain the failures they're showing.

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] The return value of allocate_recordbuf()

[Michael Paquier]

On Fri, Apr 3, 2015 at 9:57 PM, Fujii Masao  wrote:
> On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier
>  wrote:
>> On Fri, Apr 3, 2015 at 6:35 PM, Fujii Masao wrote:
>>> Regarding the second patch, you added the checks of the return value of
>>> XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
>>> uses palloc(), but don't we need to replace it with palloc_extended(), too?
>>
>> Doh, you are right. I missed three places. Attached is a new patch
>> completing the fix.
>
> Thanks for the patch! I updated two source code comments and
> changed the log message when XLogReaderAllocate returns NULL
> within XLOG_DEBUG block. Just pushed.

Yes, thanks. This looks good as is.
-- 
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] Table-level log_autovacuum_min_duration

[Michael Paquier]

On Fri, Apr 3, 2015 at 11:59 PM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>> On Fri, Apr 3, 2015 at 3:26 PM, Michael Paquier wrote:
>> > [...]
>> > Fine for me.
>>
>> And here are the correct patches. Sorry for that.
>
> Thanks, pushed.  I added one extra comment to the SIGHUP patch in the
> place where you previously had the exit.

Thanks!
-- 
Michael


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

Re: [HACKERS] Compile warnings on OSX 10.10 clang 6.0

[Tom Lane]

I wrote:
> Peter Eisentraut  writes:
>> These warnings also happen with older versions of clang.  Now idea how
>> to fix yet.  I'm thinking that clang should be fixed, because these
>> warnings are stupid.

> Yeah, they're utterly stupid; whoever put them in obviously doesn't
> have a clue about typical Makefile construction.  I wonder if next
> we'll see complaints about unnecessary -D or -I switches.

> Having said that, I did look awhile ago about how we might get rid of
> them, and it seems not easy; for starters we would need to drop the
> assumption that CFLAGS can always be included when linking.  Also,
> AFAICT -pthread sometimes *is* required when linking; so it's
> not even very obvious when to suppress the switch, even if we could
> do so without wholesale rearrangement of our FLAGS handling.

On the other hand, there's often more than one way to skin a cat.
It occurred to me that maybe we could just turn off this class of warning,
and after some experimentation I found out that
"-Wno-unused-command-line-argument" does that, at least in the version
of clang that Apple's currently shipping.

Who's for enabling that if the compiler takes it?

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] Compile warnings on OSX 10.10 clang 6.0

[Tom Lane]

Peter Eisentraut  writes:
> On 4/3/15 4:02 PM, John Gorman wrote:
>> I am getting compile warnings on OSX 10.10 from clang 6.0:
>> 
>> clang: warning: argument unused during compilation: '-pthread'
>> 
>> The 5 warnings are where we are making a -dynamiclib and
>> the -pthread argument is not necessary:
>> 
>> ./src/interfaces/libpq/
>> ./src/interfaces/ecpg/pgtypeslib/
>> ./src/interfaces/ecpg/ecpglib/
>> ./src/interfaces/ecpg/compatlib/
>> ./src/interfaces/ecpg/preproc/
>> 
>> This is interfering with using "-Wall -Werror" to catch warnings.
>> 
>> Any opinions as to whether this is worth fixing and if so
>> what the cleanest approach might be?

> These warnings also happen with older versions of clang.  Now idea how
> to fix yet.  I'm thinking that clang should be fixed, because these
> warnings are stupid.

Yeah, they're utterly stupid; whoever put them in obviously doesn't
have a clue about typical Makefile construction.  I wonder if next
we'll see complaints about unnecessary -D or -I switches.

Having said that, I did look awhile ago about how we might get rid of
them, and it seems not easy; for starters we would need to drop the
assumption that CFLAGS can always be included when linking.  Also,
AFAICT -pthread sometimes *is* required when linking; so it's
not even very obvious when to suppress the switch, even if we could
do so without wholesale rearrangement of our FLAGS handling.

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] Unused variable in hashpage.c

[Tom Lane]

Petr Jelinek  writes:
> my compiler complains about unused variable nblkno in _hash_splitbucket 
> in no-assert build. It looks like relic of commit ed9cc2b5d which 
> removed the only use of that variable besides the Assert.

Fixed, thanks!

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] Compile warnings on OSX 10.10 clang 6.0

[Peter Eisentraut]

On 4/3/15 4:02 PM, John Gorman wrote:
> I am getting compile warnings on OSX 10.10 from clang 6.0:
> 
> clang: warning: argument unused during compilation: '-pthread'
> 
> The 5 warnings are where we are making a -dynamiclib and
> the -pthread argument is not necessary:
> 
> ./src/interfaces/libpq/
> ./src/interfaces/ecpg/pgtypeslib/
> ./src/interfaces/ecpg/ecpglib/
> ./src/interfaces/ecpg/compatlib/
> ./src/interfaces/ecpg/preproc/
> 
> This is interfering with using "-Wall -Werror" to catch warnings.
> 
> Any opinions as to whether this is worth fixing and if so
> what the cleanest approach might be?

These warnings also happen with older versions of clang.  Now idea how
to fix yet.  I'm thinking that clang should be fixed, because these
warnings are stupid.


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

[HACKERS] Compile warnings on OSX 10.10 clang 6.0

[John Gorman]

Hi All

I am getting compile warnings on OSX 10.10 from clang 6.0:

clang: warning: argument unused during compilation: '-pthread'

The 5 warnings are where we are making a -dynamiclib and
the -pthread argument is not necessary:

./src/interfaces/libpq/
./src/interfaces/ecpg/pgtypeslib/
./src/interfaces/ecpg/ecpglib/
./src/interfaces/ecpg/compatlib/
./src/interfaces/ecpg/preproc/

This is interfering with using "-Wall -Werror" to catch warnings.

Any opinions as to whether this is worth fixing and if so
what the cleanest approach might be?

Thanks, John

Re: [HACKERS] Abbreviated keys for Numeric

[Andrew Gierth]

> "Robert" == Robert Haas  writes:

 >> It already does; it changes how int64 values are expected to be
 >> stored in Datum variables. _Everything_ that currently stores an
 >> int64 value in a Datum is affected.

 Robert> But this isn't a value of the SQL type "int64".  It's just a
 Robert> bit pattern that has to fit inside however big a Datum happens
 Robert> to be.

It's a bit pattern which is a signed 32-bit or 64-bit integer, computed
in an int32 or int64. Using something other than Int32GetDatum /
Int64GetDatum for it seems unnecessarily surprising.

 >> The fact that making this one low-benefit change has introduced no
 >> less than three separate bugs - the compile error with that #ifdef,
 >> the use of Int64GetDatum for NANs, and the use of Int64GetDatum for
 >> the return value of the abbreviation function should possibly be
 >> taken as a hint to how bad an idea is.

 Robert> But all of those are trivial, and the first would have been
 Robert> caught by my compiler if I weren't using such a crappy old
 Robert> compiler.  If anything that might require as much as 10 lines
 Robert> of code churn to fix is not worth doing, very little is worth
 Robert> doing.

Trivial maybe, but subtle enough that you missed them (which suggests
that others might too), and a --disable-float8-byval build of the buggy
version only fails regression by a fluke.

(This does rather suggest to me that some better regression tests for
sorting would be a good idea, possibly even including on-disk sorts.)

 >> If you're determined to go this route - over my protest - then you
 >> need to do something like define a NumericAbbrevGetDatum(x) macro
 >> and use it in place of the Int64GetDatum / Int32GetDatum ones for
 >> both NAN and the return from numeric_abbrev_convert_var.

 Robert> Patch for that attached.

That looks reasonable, though I think it could do with a comment
explaining _why_ it's defining its own macros rather than using
Int32*/Int64*. (And I wrote that before seeing Tom's message, even.)

-- 
Andrew (irc:RhodiumToad)


-- 
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] Abbreviated keys for Numeric

[Tom Lane]

I wrote:
> FWIW, I think it's sensible to define NumericAbbrevGetDatum and the
> converse, but I'd suggest you just do it like
> #define NumericAbbrevGetDatum(X) Int64GetDatum(X)
> or
> #define NumericAbbrevGetDatum(X) Int32GetDatum(X)

Oh, scratch that, I'd failed to look at the upthread messages.

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] Abbreviated keys for Numeric

[Tom Lane]

Robert Haas  writes:
> On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth
>  wrote:
>> If you're determined to go this route - over my protest - then you need
>> to do something like define a NumericAbbrevGetDatum(x) macro and use it
>> in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the
>> return from numeric_abbrev_convert_var.

> Patch for that attached.

FWIW, I think it's sensible to define NumericAbbrevGetDatum and the
converse, but I'd suggest you just do it like

#define NumericAbbrevGetDatum(X) Int64GetDatum(X)
or
#define NumericAbbrevGetDatum(X) Int32GetDatum(X)

I'm not especially a fan of reaching inside the GetDatum macros when
you don't have to.  And the code that's calling these certainly knows
that it's supplying an int64 or int32 respectively.

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] Abbreviated keys for Numeric

[Robert Haas]

On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth
 wrote:
> It already does; it changes how int64 values are expected to be stored
> in Datum variables. _Everything_ that currently stores an int64 value in
> a Datum is affected.

But this isn't a value of the SQL type "int64".  It's just a bit
pattern that has to fit inside however big a Datum happens to be.

> As I see it, you need a really good reason to override that in a
> specific case, and supporting 64-bit abbreviations on a
> --disable-float8-byval build really isn't a good reason (since 32-bit
> abbrevs work fine and such builds should be vanishingly rare anyway).

In my opinion, there is way too much crap around already just to cater
to --disable-float8-byval.  I think not adding more is a perfectly
reasonable goal.  If somebody wants to remove --disable-float8-byval
some day, I want to minimize the amount of stuff they have to change
in order to do that.  I think that keeping this off the list of stuff
that will require modification is a worthy goal.

> The fact that making this one low-benefit change has introduced no less
> than three separate bugs - the compile error with that #ifdef, the use
> of Int64GetDatum for NANs, and the use of Int64GetDatum for the return
> value of the abbreviation function should possibly be taken as a hint to
> how bad an idea is.

But all of those are trivial, and the first would have been caught by
my compiler if I weren't using such a crappy old compiler.  If
anything that might require as much as 10 lines of code churn to fix
is not worth doing, very little is worth doing.

> If you're determined to go this route - over my protest - then you need
> to do something like define a NumericAbbrevGetDatum(x) macro and use it
> in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the
> return from numeric_abbrev_convert_var.

Patch for that attached.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index dd108c8..0b11985 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -298,11 +298,13 @@ typedef struct
 
 #define NUMERIC_ABBREV_BITS (SIZEOF_DATUM * BITS_PER_BYTE)
 #if SIZEOF_DATUM == 8
-#define DatumGetNumericAbbrev(d) ((int64) d)
-#define NUMERIC_ABBREV_NAN Int64GetDatum(PG_INT64_MIN)
+#define NumericAbbrevGetDatum(X) ((Datum) SET_8_BYTES(X))
+#define DatumGetNumericAbbrev(X) ((int64) GET_8_BYTES(X))
+#define NUMERIC_ABBREV_NAN		 NumericAbbrevGetDatum(PG_INT64_MIN)
 #else
-#define DatumGetNumericAbbrev(d) ((int32) d)
-#define NUMERIC_ABBREV_NAN Int32GetDatum(PG_INT32_MIN)
+#define NumericAbbrevGetDatum(X) ((Datum) SET_4_BYTES(X))
+#define DatumGetNumericAbbrev(X) ((int32) GET_4_BYTES(X))
+#define NUMERIC_ABBREV_NAN		 NumericAbbrevGetDatum(PG_INT32_MIN)
 #endif
 
 
@@ -1883,7 +1885,7 @@ numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss)
 		addHyperLogLog(&nss->abbr_card, DatumGetUInt32(hash_uint32(tmp)));
 	}
 
-	return Int64GetDatum(result);
+	return NumericAbbrevGetDatum(result);
 }
 
 #endif   /* NUMERIC_ABBREV_BITS == 64 */
@@ -1960,7 +1962,7 @@ numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss)
 		addHyperLogLog(&nss->abbr_card, DatumGetUInt32(hash_uint32(tmp)));
 	}
 
-	return Int32GetDatum(result);
+	return NumericAbbrevGetDatum(result);
 }
 
 #endif   /* NUMERIC_ABBREV_BITS == 32 */

-- 
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] Abbreviated keys for Numeric

[Andrew Gierth]

> "Robert" == Robert Haas  writes:

 >> I don't consider it appropriate to override ./configure in this way.

 Robert> I don't see how that's overriding configure.  Commit
 Robert> 8472bf7a73487b0535c95e299773b882f7523463, which introduced
 Robert> --disable-float8-byval in 2008, states that the motivation for
 Robert> this is that it might break functions using the version 0
 Robert> calling convention.  It should not propagate, like kudzu, into
 Robert> things that having nothing to do with how values are passed to
 Robert> V0 functions.

It already does; it changes how int64 values are expected to be stored
in Datum variables. _Everything_ that currently stores an int64 value in
a Datum is affected.

As I see it, you need a really good reason to override that in a
specific case, and supporting 64-bit abbreviations on a
--disable-float8-byval build really isn't a good reason (since 32-bit
abbrevs work fine and such builds should be vanishingly rare anyway).

The fact that making this one low-benefit change has introduced no less
than three separate bugs - the compile error with that #ifdef, the use
of Int64GetDatum for NANs, and the use of Int64GetDatum for the return
value of the abbreviation function should possibly be taken as a hint to
how bad an idea is.

If you're determined to go this route - over my protest - then you need
to do something like define a NumericAbbrevGetDatum(x) macro and use it
in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the
return from numeric_abbrev_convert_var.

-- 
Andrew (irc:RhodiumToad)


-- 
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] initdb -S and tablespaces

[Alvaro Herrera]

Abhijit Menon-Sen wrote:
> At 2015-01-15 14:32:45 +0100, and...@2ndquadrant.com wrote:

> Patch attached.
> 
> Changes:
> 
> 1. Renamed perform_fsync to fsync_recursively (otherwise it would read
>"fsync_pgdata(pg_data)")

Okay, but as far as I can tell this function is very specific to
PGDATA; you couldn't use it in any other directory (or pg_tblspc would
be missing and that would cause an error, right?)  Therefore I think it
would make sense to have the name reflect this; maybe
fsync_datadir_recursively(data_directory)
or
fsync_pgdata_recursively(data_directory)
would work?  But then, since the name is already telling us that it's
all about PGDATA, maybe we can remove the "recursively" part of the
name?  Not sure about any of this; other opinions?

I also noticed that walkdir() thinks it is completely agnostic on what
the passed action is; except that the comment at the bottom talks about
how fsync on directories is important, which seems out of place.

I wonder about walktblspc_links() too.  Seems to me that that function
is pretty much the same as walkdir(); would it work to add a flag to the
latter to change the behavior in whatever way needs to be changed, and
remove the former?  Hmm ... Actually, since surely we must follow
symlinks everywhere, why do we have to do this separately for pg_tblspc?
Shouldn't that link-following occur automatically when walking PGDATA in
the first place?


> 2. Added ControlData->fsync_disabled to record whether fsync was ever
>disabled while the server was running (tested in CreateCheckPoint)
> 3. Run fsync_recursively at startup only if fsync is enabled
> 4. Run it if we're doing crash recovery, or fsync was disabled
> 5. Use pg_flush_data in pre_sync_fname
> 6. Issue fsync on directories too
> 7. Tested that it works if pg_xlog is a symlink (no changes).
> 
> (In short, everything you mentioned in your earlier mail.)
> 
> Note that I set ControlData->fsync_disabled to false in BootstrapXLOG,
> but it gets set to true during a later CreateCheckPoint(). This means
> we run fsync again at startup after initdb. I'm not sure what to do
> about that.

This all looks reasonable to me.  I just noticed, though, that
the fd.c routines test enableFsync and do nothing if it's not enabled;
but fsync_recursively goes to all the trouble of doing stuff even if
disabled, and the actions are skipped later; the enableFsync check is
then responsibility of the caller.  This seems a bit prone to later
confusion.  Maybe fsync_recursively should Assert() that it's only being
called with enableFsync=on; or perhaps we can have it return early if
it's unset.

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


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

Re: [HACKERS] a fast bloat measurement tool (was Re: Measuring relation free space)

[Abhijit Menon-Sen]

At 2015-03-31 22:43:49 +0530, a...@2ndquadrant.com wrote:
>
> I'm just posting this WIP patch where I've renamed fastbloat to
> pgstatbloat as suggested by Tomas, and added in the documentation, and
> so on.

Here's the revised version that also adds the count of RECENTLY_DEAD and
INSERT/DELETE_IN_PROGRESS tuples to the call to vac_estimate_reltuples.

Tomas, I agree that the build_output_tuple function could use cleaning
up, but it's the same as the corresponding code in pgstattuple, and it
seems to me reasonable to clean both up together in a separate patch.

-- Abhijit
>From d11b84627438fca12955dfad77042be0a27c9acc Mon Sep 17 00:00:00 2001
From: Abhijit Menon-Sen 
Date: Fri, 26 Dec 2014 12:37:13 +0530
Subject: Add pgstatbloat to pgstattuple

---
 contrib/pgstattuple/Makefile   |   4 +-
 contrib/pgstattuple/pgstatbloat.c  | 340 +
 contrib/pgstattuple/pgstattuple--1.2--1.3.sql  |  18 ++
 .../{pgstattuple--1.2.sql => pgstattuple--1.3.sql} |  18 +-
 contrib/pgstattuple/pgstattuple.control|   2 +-
 doc/src/sgml/pgstattuple.sgml  | 135 
 6 files changed, 513 insertions(+), 4 deletions(-)
 create mode 100644 contrib/pgstattuple/pgstatbloat.c
 create mode 100644 contrib/pgstattuple/pgstattuple--1.2--1.3.sql
 rename contrib/pgstattuple/{pgstattuple--1.2.sql => pgstattuple--1.3.sql} (73%)

diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index 862585c..d7d27a5 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -1,10 +1,10 @@
 # contrib/pgstattuple/Makefile
 
 MODULE_big	= pgstattuple
-OBJS		= pgstattuple.o pgstatindex.o $(WIN32RES)
+OBJS		= pgstattuple.o pgstatindex.o pgstatbloat.o $(WIN32RES)
 
 EXTENSION = pgstattuple
-DATA = pgstattuple--1.2.sql pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.3.sql pgstattuple--1.2--1.3.sql pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
 PGFILEDESC = "pgstattuple - tuple-level statistics"
 
 REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/pgstatbloat.c b/contrib/pgstattuple/pgstatbloat.c
new file mode 100644
index 000..2dd1900
--- /dev/null
+++ b/contrib/pgstattuple/pgstatbloat.c
@@ -0,0 +1,340 @@
+/*
+ * contrib/pgstattuple/pgstatbloat.c
+ *
+ * Abhijit Menon-Sen 
+ * Portions Copyright (c) 2001,2002	Tatsuo Ishii (from pgstattuple)
+ *
+ * Permission to use, copy, modify, and distribute this software and
+ * its documentation for any purpose, without fee, and without a
+ * written agreement is hereby granted, provided that the above
+ * copyright notice and this paragraph and the following two
+ * paragraphs appear in all copies.
+ *
+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE TO ANY PARTY FOR DIRECT,
+ * INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES, INCLUDING
+ * LOST PROFITS, ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS
+ * DOCUMENTATION, EVEN IF THE UNIVERSITY OF CALIFORNIA HAS BEEN ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THE AUTHOR SPECIFICALLY DISCLAIMS ANY WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE.  THE SOFTWARE PROVIDED HEREUNDER IS ON AN "AS
+ * IS" BASIS, AND THE AUTHOR HAS NO OBLIGATIONS TO PROVIDE MAINTENANCE,
+ * SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
+ */
+
+#include "postgres.h"
+
+#include "access/visibilitymap.h"
+#include "access/transam.h"
+#include "access/xact.h"
+#include "access/multixact.h"
+#include "access/htup_details.h"
+#include "catalog/namespace.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "storage/bufmgr.h"
+#include "storage/freespace.h"
+#include "storage/procarray.h"
+#include "storage/lmgr.h"
+#include "utils/builtins.h"
+#include "utils/tqual.h"
+#include "commands/vacuum.h"
+
+PG_FUNCTION_INFO_V1(pgstatbloat);
+
+/*
+ * tuple_percent, dead_tuple_percent and free_percent are computable,
+ * so not defined here.
+ */
+typedef struct pgstatbloat_output_type
+{
+	uint64		table_len;
+	uint64		tuple_count;
+	uint64		misc_count;
+	uint64		tuple_len;
+	uint64		dead_tuple_count;
+	uint64		dead_tuple_len;
+	uint64		free_space;
+	uint64		total_pages;
+	uint64		scanned_pages;
+} pgstatbloat_output_type;
+
+static Datum build_output_type(pgstatbloat_output_type *stat,
+			   FunctionCallInfo fcinfo);
+static Datum pgstatbloat_heap(Relation rel, FunctionCallInfo fcinfo);
+
+/*
+ * build a pgstatbloat_output_type tuple
+ */
+static Datum
+build_output_type(pgstatbloat_output_type *stat, FunctionCallInfo fcinfo)
+{
+#define NCOLUMNS	10
+#define NCHARS		32
+
+	HeapTuple	tuple;
+	char	   *values[NCOLUMNS];
+	char		values_buf[NCOLUMNS][NCHARS];
+	int			i;
+	double		tuple_percent;
+	double		dead_tuple_percent;
+	double		free_percent;	/* free/reusable space in % */
+	double		scanned_percent;
+	TupleDesc	tupdesc;
+	AttInMetadata *attinmeta;
+
+	/* Build a tuple 

Re: [HACKERS] Table-level log_autovacuum_min_duration

[Alvaro Herrera]

Michael Paquier wrote:
> On Fri, Apr 3, 2015 at 3:26 PM, Michael Paquier wrote:
> > [...]
> > Fine for me.
> 
> And here are the correct patches. Sorry for that.

Thanks, pushed.  I added one extra comment to the SIGHUP patch in the
place where you previously had the exit.

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


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

Re: [HACKERS] Possibly a typo in expand_inherited_rtentry()

[Tom Lane]

Amit Langote  writes:
> Attached does:
> -/* Open rel if needed; we already have required locks */
> +/* Open rel if needed; we already have acquired locks */

> Does that make sense?

No, not particularly.  It could be made to say "the locks we require"
but I don't find anything wrong with the existing wording (and I'll
bet there is similar wording in many other places).

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] Abbreviated keys for Numeric

[Robert Haas]

On Fri, Apr 3, 2015 at 9:06 AM, Andrew Gierth
 wrote:
>> "Robert" == Robert Haas  writes:
>  >> No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the
>  >> definitions of Int64GetDatum / DatumGetInt64 are conditional on. As
>  >> you have it now, it'll break if you build with
>  >> --disable-float8-byval on a 64bit platform.
>
>  Robert> I'd prefer to avoid that by avoiding the use of those macros in
>  Robert> this code path rather than by leaving the top 32 bits of the
>  Robert> Datum unused
>
> I don't consider it appropriate to override ./configure in this way.

I don't see how that's overriding configure.  Commit
8472bf7a73487b0535c95e299773b882f7523463, which introduced
--disable-float8-byval in 2008, states that the motivation for this is
that it might break functions using the version 0 calling convention.
It should not propagate, like kudzu, into things that having nothing
to do with how values are passed to V0 functions.  And as far as I can
see, the algorithm that we use to create an abbreviated key for
numeric is entirely decoupled from that question, because none of the
datums were are building here will ever get passed to one.

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


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

Re: [HACKERS] PATCH: adaptive ndistinct estimator v4

[Greg Stark]

> The simple workaround for this was adding a fallback to GEE when f[1] or
f[2] is 0. GEE is another estimator described in the paper, behaving much
better in those cases.

For completeness, what's the downside in just always using GEE?

Re: [HACKERS] Abbreviated keys for Numeric

[Andrew Gierth]

> "Robert" == Robert Haas  writes:

 >> No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the
 >> definitions of Int64GetDatum / DatumGetInt64 are conditional on. As
 >> you have it now, it'll break if you build with
 >> --disable-float8-byval on a 64bit platform.

 Robert> I'd prefer to avoid that by avoiding the use of those macros in
 Robert> this code path rather than by leaving the top 32 bits of the
 Robert> Datum unused

I don't consider it appropriate to override ./configure in this way.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Abbreviated keys for text cost model fix

[Peter Geoghegan]

On Fri, Apr 3, 2015 at 1:49 PM, Robert Haas  wrote:
> Committed.  For future reference, I'd prefer to have things like this
> added to the next CF rather than no CF at all.

I'll bear that in mind. Thanks.


-- 
Peter Geoghegan


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

Re: [HACKERS] Abbreviated keys for Numeric

[Robert Haas]

On Thu, Apr 2, 2015 at 10:41 PM, Andrew Gierth
 wrote:
>  Robert> - Changed some definitions to depend on SIZEOF_DATUM rather
>  Robert> than USE_FLOAT8_BYVAL.  Hopefully I didn't muff this; please
>  Robert> check it.
>
> No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the
> definitions of Int64GetDatum / DatumGetInt64 are conditional on. As you
> have it now, it'll break if you build with --disable-float8-byval on a
> 64bit platform.

I'd prefer to avoid that by avoiding the use of those macros in this
code path rather than by leaving the top 32 bits of the Datum unused
in that configuration.  I tried to get there by using a cast for
DatumGetNumericAbbrev(), but I forgot to update the NAN case, so at
least this much is probably needed:

--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -299,10 +299,10 @@ typedef struct
 #define NUMERIC_ABBREV_BITS (SIZEOF_DATUM * BITS_PER_BYTE)
 #if SIZEOF_DATUM == 8
 #define DatumGetNumericAbbrev(d) ((int64) d)
-#define NUMERIC_ABBREV_NAN Int64GetDatum(PG_INT64_MIN)
+#define NUMERIC_ABBREV_NAN ((Datum) PG_INT64_MIN)
 #else
 #define DatumGetNumericAbbrev(d) ((int32) d)
-#define NUMERIC_ABBREV_NAN Int32GetDatum(PG_INT32_MIN)
+#define NUMERIC_ABBREV_NAN ((Datum) PG_INT32_MIN)
 #endif

What other risks do you see?

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


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

Re: [HACKERS] Auditing extension for PostgreSQL (Take 2)

[David Steele]

On 4/3/15 3:59 AM, Sawada Masahiko wrote:
> On Thu, Apr 2, 2015 at 2:46 AM, David Steele  wrote:
>> Let me know if you see any other issues.
>>
> 
> I pulled HEAD, and then tried to compile source code after applied
> following "deparsing utility command patch" without #0001 and #0002.
> (because these two patches are already pushed)
> 
> 
> But I could not use new pg_audit patch due to compile error (that
> deparsing utility command patch might have).
> I think I'm missing something, but I'm not sure.
> Could you tell me which patch should I apply before compiling pg_audit?

When Alvaro posted his last patch set he recommended applying them to
b3196e65:

http://www.postgresql.org/message-id/20150325175954.gl3...@alvh.no-ip.org

Applying the 3/25 deparse patches onto b3196e65 (you'll see one trailing
space error) then applying pg_audit-v6.patch works fine.

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] The return value of allocate_recordbuf()

[Fujii Masao]

On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier
 wrote:
> On Fri, Apr 3, 2015 at 6:35 PM, Fujii Masao wrote:
>> Regarding the second patch, you added the checks of the return value of
>> XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
>> uses palloc(), but don't we need to replace it with palloc_extended(), too?
>
> Doh, you are right. I missed three places. Attached is a new patch
> completing the fix.

Thanks for the patch! I updated two source code comments and
changed the log message when XLogReaderAllocate returns NULL
within XLOG_DEBUG block. Just pushed.

Regards,

-- 
Fujii Masao


-- 
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] Abbreviated keys for text cost model fix

[Robert Haas]

On Mon, Feb 23, 2015 at 7:44 PM, Peter Geoghegan  wrote:
> On Mon, Feb 23, 2015 at 4:41 PM, Tomas Vondra
>  wrote:
>> Are you going to add this into the CF? Would be nice to get this into
>> 9.5. Strictly speaking it should go to 2015-06 I guess, but I'd consider
>> it part of one of the existing sortsupport patches.
>
> It's a bugfix, IMV. I guess I should add it to the current CF, but I
> think I might be forbidden from doing so as a non-CF-manager...

Committed.  For future reference, I'd prefer to have things like this
added to the next CF rather than no CF at all.

I'm not sure if you've got all the details right here, but the concept
makes sense to me: if we're going to give up, we should do it early.
Doing it later throws away more work and is therefore riskier.

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


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

Re: [HACKERS] Cube extension kNN support

[Alexander Korotkov]

On Thu, Mar 12, 2015 at 8:43 PM, Stas Kelvich 
wrote:

> Documentation along with style fix.
>

Since we change the interface of extension we have to change it version and
create a migration script.
E.g. you have to rename cube--1.0.sql to cube--1.1.sql and create
cube--1.0--1.1.sql to migrate the old version.

+ -- Alias for backword compatibility
  CREATE FUNCTION cube_distance(cube, cube)
  RETURNS float8
+ AS 'MODULE_PATHNAME', 'distance_euclid'
+ LANGUAGE C IMMUTABLE STRICT;

For backward compatibility it would be better to keep the old name of
cube_distance so that extension with old definition could work with new
binary.

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort

[Peter Geoghegan]

On Fri, Apr 3, 2015 at 1:17 PM, Stephen Frost  wrote:
>> but even I'm not willing to
>> expend the amount of ink and emotional energy you have on whether a
>> variable that holds +1, 0, or -1 ought to be declared as "int" or
>> "int32".  Does it matter?  Yeah.  Is it worth this much argument?  No.
>
> My only comment will be on this very minor aspect (and I'm quite
> agnostic as to what is decided, particularly as I haven't read the patch
> at all), but, should we consider an enum (generically) for such cases?
> If that's truly the extent of possible values, and anything else is an
> error, then at least if I was writing DDL to support this, I'd use an
> enum, maybe a domain, or a CHECK constraint (though I'd likely feel
> better about the enum or domain approach).

It isn't the case that an enum can support it. Some comparators will
return -5 rather than -1 (as with C-style comparators in general,
sometimes comparisons can be implemented using subtraction and things
like that).

-- 
Peter Geoghegan


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

Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> I'm about as much
> of a stickler for the details as you will find on this mailing list,
> or possibly, in the observable universe,

This made me laugh. :)

> but even I'm not willing to
> expend the amount of ink and emotional energy you have on whether a
> variable that holds +1, 0, or -1 ought to be declared as "int" or
> "int32".  Does it matter?  Yeah.  Is it worth this much argument?  No.

My only comment will be on this very minor aspect (and I'm quite
agnostic as to what is decided, particularly as I haven't read the patch
at all), but, should we consider an enum (generically) for such cases?
If that's truly the extent of possible values, and anything else is an
error, then at least if I was writing DDL to support this, I'd use an
enum, maybe a domain, or a CHECK constraint (though I'd likely feel
better about the enum or domain approach).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort

[Peter Geoghegan]

On Fri, Apr 3, 2015 at 1:07 PM, Robert Haas  wrote:
> I'm about as much
> of a stickler for the details as you will find on this mailing list,
> or possibly, in the observable universe, but even I'm not willing to
> expend the amount of ink and emotional energy you have on whether a
> variable that holds +1, 0, or -1 ought to be declared as "int" or
> "int32".  Does it matter?  Yeah.  Is it worth this much argument?  No.


I haven't really spent very much time arguing about this point at all,
and intend to spend no more time on it. It's up to you.

-- 
Peter Geoghegan


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

Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort

[Robert Haas]

On Thu, Apr 2, 2015 at 7:02 PM, Peter Geoghegan  wrote:
> On Thu, Apr 2, 2015 at 11:21 PM, Robert Haas  wrote:
>>> The changes that Andrew
>>> took issue with are utterly insignificant.
>>
>> Great.  Then you will be utterly indifferent to which version gets committed.
>
> *shrug*
>
> You were the one that taught me to be bureaucratically minded about
> keeping code consistent at this fine a level. I think it's odd that
> you of all people are opposing me on this point, but whatever.

Sure, consistency is important.  But sometimes there is more than one
thing that you can choose to be consistent with.  IIUC, you're
complaining because somebody assigned the return value of a function
to a variable whose type matches the function's return type, rather
than assigning it to a variable of the same mismatching type used in
parallel code elsewhere.  Which form of consistency to aim for in such
cases is fundamentally a judgement call.  I'll have another look over
the patch and maybe I'll come around to your point of view, but you
don't seem very willing to concede the point that intelligent people
could disagree over what is most consistent here.  I'm about as much
of a stickler for the details as you will find on this mailing list,
or possibly, in the observable universe, but even I'm not willing to
expend the amount of ink and emotional energy you have on whether a
variable that holds +1, 0, or -1 ought to be declared as "int" or
"int32".  Does it matter?  Yeah.  Is it worth this much argument?  No.

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


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

Re: [HACKERS] libpq's multi-threaded SSL callback handling is busted

[Jan Urbański]

Peter Eisentraut writes:
> On 4/2/15 4:32 AM, Jan Urbański wrote:
>> 
>> Peter Eisentraut writes:
>>> I don't think this patch would actually fix the problem that was
>>> described after the original bug report
>>> (http://www.postgresql.org/message-id/5436991b.5020...@vmware.com),
>>> namely that another thread acquires a lock while the libpq callbacks are
>>> set and then cannot release the lock if libpq has been shut down in the
>>> meantime.
>> 
>> I did test both the Python and the PHP repro scripts and the patch fixed both
>> the deadlock and the segfault.
>> 
>> What happens is that Python (for instance) stops over the callback
>> unconditionally. So when libpq gets unloaded, it sees that the currently set
>> callback is no the one it originally set and refrains from NULLing it.
>
> So this works because Python is just as broken as libpq right now.  What
> happens if Python is fixed as well?  Then we'll have the problem I
> described above.

Well, not really, libpq sets and unsets the callbacks every time an SSL
connection is opened and closed. Python sets the callbacks once when the ssl
module is imported and never touches them again.

Arguably, it should set them even earlier, but it's still saner than
flip-flopping them. AFAIK you can't "unload" Python, so setting the callback
and keeping it there is a sound strategy.

The change I'm proposing is not to set the callback in libpq if one is already
set and not remove it if the one that's set is not libpq's. That's as sane as
it gets.

With multiple libraries wanting to use OpenSSL in threaded code, the mechanism
seems to be "last one wins". It doesn't matter *who's* callbacks are used, as
long as they're there and they stay there and that's Python's stance. This
doesn't work if you want to be able to unload the library, so the next best
thing is not touching the callback if someone else set his in the meantime.

What's broken is OpenSSL for offering such a bad way of dealing with
concurrency.

To reiterate: I recognise that not handling the callbacks is not the right
answer. But not stomping on callbacks others might have set sounds like a
minimal and safe improvement.

Cheers,
Jan


-- 
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] EvalPlanQual behaves oddly for FDW queries involving system columns

[Etsuro Fujita]

On 2015/03/13 0:50, Tom Lane wrote:

I think the real fix as far as postgres_fdw is concerned is in fact
to let it adopt a different ROW_MARK strategy, since it has meaningful
ctid values.  However, that is not a one-size-fits-all answer.



The tableoid problem can be fixed much less invasively as per the attached
patch.  I think that we should continue to assume that ctid is not
meaningful (and hence should read as (4294967295,0)) in FDWs that use
ROW_MARK_COPY, and press forward on fixing the locking issues for
postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to
that.  That would also cause ctid to read properly for rows from
postgres_fdw.


To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like 
to propose the following FDW APIs:


RowMarkType
GetForeignRowMarkType(Oid relid,
  LockClauseStrength strength);

Decide which rowmark type to use for a foreign table (that has strength 
= LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY.  (For now, the 
second argument takes LCS_NONE only, but is intended to be used for the 
possible extension to the other cases.)  This is called during 
select_rowmark_type() in the planner.


void
BeginForeignFetch(EState *estate,
  ExecRowMark *erm,
  List *fdw_private,
  int eflags);

Begin a remote fetch. This is called during InitPlan() in the executor.

HeapTuple
ExecForeignFetch(EState *estate,
 ExecRowMark *erm,
 ItemPointer tupleid);

Re-fetch the specified tuple.  This is called during 
EvalPlanQualFetchRowMarks() in the executor.


void
EndForeignFetch(EState *estate,
ExecRowMark *erm);

End a remote fetch.  This is called during ExecEndPlan() in the executor.

And I'd also like to propose to add a table/server option, 
row_mark_reference, to postgres_fdw.  When a user sets the option to 
true for eg a foreign table, ROW_MARK_REFERENCE will be used for the 
table, not ROW_MARK_COPY.


Attached is a WIP patch, which contains no docs/regression tests.

It'd be appreciated if anyone could send back any comments earlier.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/option.c
--- b/contrib/postgres_fdw/option.c
***
*** 105,111  postgres_fdw_validator(PG_FUNCTION_ARGS)
  		 * Validate option value, when we can do so without any context.
  		 */
  		if (strcmp(def->defname, "use_remote_estimate") == 0 ||
! 			strcmp(def->defname, "updatable") == 0)
  		{
  			/* these accept only boolean values */
  			(void) defGetBoolean(def);
--- 105,112 
  		 * Validate option value, when we can do so without any context.
  		 */
  		if (strcmp(def->defname, "use_remote_estimate") == 0 ||
! 			strcmp(def->defname, "updatable") == 0 ||
! 			strcmp(def->defname, "row_mark_reference") == 0)
  		{
  			/* these accept only boolean values */
  			(void) defGetBoolean(def);
***
*** 153,158  InitPgFdwOptions(void)
--- 154,162 
  		/* updatable is available on both server and table */
  		{"updatable", ForeignServerRelationId, false},
  		{"updatable", ForeignTableRelationId, false},
+ 		/* row_mark_reference is available on both server and table */
+ 		{"row_mark_reference", ForeignServerRelationId, false},
+ 		{"row_mark_reference", ForeignTableRelationId, false},
  		{NULL, InvalidOid, false}
  	};
  
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 124,129  enum FdwModifyPrivateIndex
--- 124,144 
  };
  
  /*
+  * Similarly, this enum describes what's kept in the fdw_private list for
+  * a PlanRowMark node referencing a postgres_fdw foreign table.  We store:
+  *
+  * 1) SELECT statement text to be sent to the remote server
+  * 2) Integer list of attribute numbers retrieved by SELECT
+  */
+ enum FdwFetchPrivateIndex
+ {
+ 	/* SQL statement to execute remotely (as a String node) */
+ 	FdwFetchPrivateSelectSql,
+ 	/* Integer list of attribute numbers retrieved by SELECT */
+ 	FdwFetchPrivateRetrievedAttrs
+ };
+ 
+ /*
   * Execution state of a foreign scan using postgres_fdw.
   */
  typedef struct PgFdwScanState
***
*** 186,191  typedef struct PgFdwModifyState
--- 201,230 
  } PgFdwModifyState;
  
  /*
+  * Execution state of a foreign fetch operation.
+  */
+ typedef struct PgFdwFetchState
+ {
+ 	Relation	rel;			/* relcache entry for the foreign table */
+ 	AttInMetadata *attinmeta;	/* attribute datatype conversion metadata */
+ 
+ 	/* for remote query execution */
+ 	PGconn	   *conn;			/* connection for the fetch */
+ 	char	   *p_name;			/* name of prepared statement, if created */
+ 
+ 	/* extracted fdw_private data */
+ 	char	   *query;			/* text of SELECT command */
+ 	List	   *retrieved_attrs;	/* attr numbers retrieved by SELECT */
+ 
+ 	/* info about parameters for prepared statement */
+ 	int			p_nums;			/* number of parameters to transmit */
+ 	FmgrInfo   *p_flinfo;		/* output conversion funct

Re: [HACKERS] The return value of allocate_recordbuf()

[Michael Paquier]

On Fri, Apr 3, 2015 at 6:35 PM, Fujii Masao wrote:
> Regarding the second patch, you added the checks of the return value of
> XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
> uses palloc(), but don't we need to replace it with palloc_extended(), too?

Doh, you are right. I missed three places. Attached is a new patch
completing the fix.
-- 
Michael
From 292012c19805777cf17443eccd9b4ef05c5f42ec Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 3 Apr 2015 20:29:39 +0900
Subject: [PATCH] Fix error handling of XLogReaderAllocate in case of OOM

Similarly to previous fix 9b8d478, commit 2c03216 has switched
XLogReaderAllocate() to use a set of palloc calls instead of malloc,
causing any callers of this function to fail with an error instead of
receiving a NULL pointer in case of out-of-memory error. Fix this by
using palloc_extended with MCXT_ALLOC_NO_OOM that will safely return
NULL in case of an OOM, making this routine compatible with previous
major versions.
---
 src/backend/access/transam/xlogreader.c   | 23 ---
 src/backend/replication/logical/logical.c |  5 +
 src/bin/pg_rewind/parsexlog.c |  6 ++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index ffdc975..9047805 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -66,7 +66,11 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
 {
 	XLogReaderState *state;
 
-	state = (XLogReaderState *) palloc0(sizeof(XLogReaderState));
+	state = (XLogReaderState *)
+		palloc_extended(sizeof(XLogReaderState),
+		MCXT_ALLOC_NO_OOM | MCXT_ALLOC_ZERO);
+	if (!state)
+		return NULL;
 
 	state->max_block_id = -1;
 
@@ -77,14 +81,27 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
 	 * isn't guaranteed to have any particular alignment, whereas palloc()
 	 * will provide MAXALIGN'd storage.
 	 */
-	state->readBuf = (char *) palloc(XLOG_BLCKSZ);
+	state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
+			  MCXT_ALLOC_NO_OOM);
+	if (!state->readBuf)
+	{
+		pfree(state);
+		return NULL;
+	}
 
 	state->read_page = pagereadfunc;
 	/* system_identifier initialized to zeroes above */
 	state->private_data = private_data;
 	/* ReadRecPtr and EndRecPtr initialized to zeroes above */
 	/* readSegNo, readOff, readLen, readPageTLI initialized to zeroes above */
-	state->errormsg_buf = palloc(MAX_ERRORMSG_LEN + 1);
+	state->errormsg_buf = palloc_extended(MAX_ERRORMSG_LEN + 1,
+		  MCXT_ALLOC_NO_OOM);
+	if (!state->errormsg_buf)
+	{
+		pfree(state->readBuf);
+		pfree(state);
+		return NULL;
+	}
 	state->errormsg_buf[0] = '\0';
 
 	/*
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 30baa45..774ebbc 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -163,6 +163,11 @@ StartupDecodingContext(List *output_plugin_options,
 	ctx->slot = slot;
 
 	ctx->reader = XLogReaderAllocate(read_page, ctx);
+	if (!ctx->reader)
+		ereport(ERROR,
+(errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+
 	ctx->reader->private_data = ctx;
 
 	ctx->reorder = ReorderBufferAllocate();
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 0787ca1..3cf96ab 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -70,6 +70,8 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, TimeLineID tli,
 	private.datadir = datadir;
 	private.tli = tli;
 	xlogreader = XLogReaderAllocate(&SimpleXLogPageRead, &private);
+	if (xlogreader == NULL)
+		pg_fatal("out of memory");
 
 	do
 	{
@@ -121,6 +123,8 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, TimeLineID tli)
 	private.datadir = datadir;
 	private.tli = tli;
 	xlogreader = XLogReaderAllocate(&SimpleXLogPageRead, &private);
+	if (xlogreader == NULL)
+		pg_fatal("out of memory");
 
 	record = XLogReadRecord(xlogreader, ptr, &errormsg);
 	if (record == NULL)
@@ -171,6 +175,8 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, TimeLineID tli,
 	private.datadir = datadir;
 	private.tli = tli;
 	xlogreader = XLogReaderAllocate(&SimpleXLogPageRead, &private);
+	if (xlogreader == NULL)
+		pg_fatal("out of memory");
 
 	searchptr = forkptr;
 	for (;;)
-- 
2.3.5


-- 
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] EvalPlanQual behaves oddly for FDW queries involving system columns

[Etsuro Fujita]

On 2015/03/25 4:56, Tom Lane wrote:
> Etsuro Fujita  writes:
>> Let me explain further.  Here is the comment in ExecOpenScanRelation:
> 
>>* Determine the lock type we need.  First, scan to see if target
>> relation
>>* is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
>>* relation.  In either of those cases, we got the lock already.
> 
>> I think this is not true for foreign tables selected FOR UPDATE/SHARE,
>> which have markType = ROW_MARK_COPY, because such foreign tables don't
>> get opened/locked by InitPlan.  Then such foreign tables don't get
>> locked by neither of InitPlan nor ExecOpenScanRelation.  I think this is
>> a bug.
> 
> You are right.  I think it may not matter in practice, but if the executor
> is taking its own locks here then it should not overlook ROW_MARK_COPY
> cases.
> 
>> To fix it, I think we should open/lock such foreign tables at
>> InitPlan as the original patch does.
> 
> I still don't like that; InitPlan is not doing something that would
> require physical table access.  The right thing is to fix
> ExecOpenScanRelation's idea of whether InitPlan took a lock or not,
> which I've now done.

OK, thanks.

Best regards,
Etsuro Fujita


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

Re: [HACKERS] The return value of allocate_recordbuf()

[Fujii Masao]

On Fri, Apr 3, 2015 at 2:30 PM, Michael Paquier
 wrote:
> On Fri, Apr 3, 2015 at 12:56 PM, Fujii Masao wrote:
>> The first patch looks good to me basically. But I have one comment:
>> shouldn't we expose pg_malloc_extended as a global function like
>> we did pg_malloc? Some frontends might need to use it in the future.
>
> Yes, it makes sense as the other functions do it too. So I refactored
> the patch and defined a new static inline routine,
> pg_malloc_internal(), that all the other functions call to reduce the
> temperature in this code path that I suppose can become quite hot even
> for frontends. In the second patch I added as well what was needed for
> pg_rewind.

Thanks for updating the patches!
I pushed the first and a part of the second patch.

Regarding the second patch, you added the checks of the return value of
XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
uses palloc(), but don't we need to replace it with palloc_extended(), too?

Regards,

-- 
Fujii Masao


-- 
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] Auditing extension for PostgreSQL (Take 2)

[Sawada Masahiko]

On Thu, Apr 2, 2015 at 2:46 AM, David Steele  wrote:
> Hi Sawada,
>
> On 3/25/15 9:24 AM, David Steele wrote:
>> On 3/25/15 7:46 AM, Sawada Masahiko wrote:
>>> 2.
>>> I got ERROR when executing function uses cursor.
>>>
>>> 1) create empty table (hoge table)
>>> 2) create test function as follows.
>>>
>>> create function test() returns int as $$
>>> declare
>>> cur1 cursor for select * from hoge;
>>> tmp int;
>>> begin
>>> open cur1;
>>> fetch cur1 into tmp;
>>>return tmp;
>>> end$$
>>> language plpgsql ;
>>>
>>> 3) execute test function (got ERROR)
>>> =# select test();
>>> LOG:  AUDIT: SESSION,6,1,READ,SELECT,,,selecT test();
>>> LOG:  AUDIT: SESSION,6,2,FUNCTION,EXECUTE,FUNCTION,public.test,selecT 
>>> test();
>>> LOG:  AUDIT: SESSION,6,3,READ,SELECT,,,select * from hoge
>>> CONTEXT:  PL/pgSQL function test() line 6 at OPEN
>>> ERROR:  pg_audit stack is already empty
>>> STATEMENT:  selecT test();
>>>
>>> It seems like that the item in stack is already freed by deleting
>>> pg_audit memory context (in MemoryContextDelete()),
>>> before calling stack_pop in dropping of top-level Portal.
>
> This has been fixed and I have attached a new patch.  I've seen this
> with cursors before where the parent MemoryContext is freed before
> control is returned to ProcessUtility.  I think that's strange behavior
> but there's not a lot I can do about it.
>

Thank you for updating the patch!

> The code I put in to deal with this situation was not quite robust
> enough so I had to harden it a bit more.
>
> Let me know if you see any other issues.
>

I pulled HEAD, and then tried to compile source code after applied
following "deparsing utility command patch" without #0001 and #0002.
(because these two patches are already pushed)


But I could not use new pg_audit patch due to compile error (that
deparsing utility command patch might have).
I think I'm missing something, but I'm not sure.
Could you tell me which patch should I apply before compiling pg_audit?

Regards,

---
Sawada Masahiko


-- 
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] Possibly a typo in expand_inherited_rtentry()

[Amit Langote]

On 03-04-2015 PM 03:58, Amit Langote wrote:
>  Index   childRTindex;
>  AppendRelInfo *appinfo;
> 
> -/* Open rel if needed; we already have required locks */
> +/* Open rel if needed; we already have acquired locks */
>  if (childOID != parentOID)
>  newrelation = heap_open(childOID, NoLock);

Though, it may be that "required" is to imply "acquired".

Thanks,
Amit



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