Re: [HACKERS] Table-level log_autovacuum_min_duration
On Fri, Apr 3, 2015 at 5:57 AM, Alvaro Herrera wrote:
You added this in the worker loop processing each table:
/*
* Check for config changes before processing each collected
table.
*/
if (got_SIGHUP)
{
got_SIGHUP = false;
ProcessConfigFile(PGC_SIGHUP);
/* shutdown requested in config file? */
if (!AutoVacuumingActive())
break;
}
I think bailing out if autovac is disabled is a good idea; for instance,
if this is a for-wraparound vacuum, we should keep running. My first
reaction is that this part of the test ought to be moved down a
screenful or so, until we've ran the recheck step and we have the
for-wraparound flag in hand; only bail out if that one is not set. But
on the other hand, maybe the worker will process some tables that are
not for-wraparound in between some other tables that are, so that might
turn out to be a bad idea as well. (Though I'm not 100% that it works
that way; consider commit f51ead09df for instance.) Maybe the test to
use for this is something along the lines of if autovacuum was enabled
before and is no longer enabled, bail out, otherwise keep running.
This implies that we need to keep track of whether autovac was enabled
at the start of the worker run.
I did consider the case of wraparound but came to the conclusion that
bailing out as fast as possible was the idea. But well, I guess that
we could play it safe and not add this check after all. The main
use-case of this change is now to be able to do re-balancing of the
cost parameters. We could still decide later if a worker could bail
out earlier or not, depending on what.
Another thing, but not directly related to this patch: while looking at
the doc changes, I noticed that we don't have index entries for the
reloptions we have; for instance, the word fillfactor doesn't appear
in the bookindex.html page at all. Having these things all crammed in
the CREATE TABLE page seems somewhat poor. I think we could improve on
this by having a listing of settable parameters in a separate section,
and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter
link to that; we could also have index entries for these items.
Of course, the simpler fix is to just add a few indexterm tags.
This sounds like a good idea, and this refactoring would meritate a
different patch and a different thread. I guess that it should be a
new section in Server Configuration then, named Relation Options,
with two different subsections for index options and table options.
As a note, there is no point to Assert(foo != NULL) tests when foo is
later dereferenced in the same routine: the crash is going to happen
anyway at the dereference, so it's just a LOC uselessly wasted.
Check. I still think that it is useful in this case though... But removed.
I think this could use some wordsmithing; I didn't like listing the
parameters explicitely, and Jaime Casanova suggested not using the terms
inherit and parent table in this context. So I came up with this:
Note that the TOAST table uses the parameter values defined for the main
table, for each parameter applicable to TOAST tables and for which no
value is set in the TOAST table itself.
Fine for me.
--
Michael
From f43fc9a5a5e0ffb246782010b8860829b8304593 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Fri, 3 Apr 2015 14:21:12 +0900
Subject: [PATCH 1/2] Add palloc_extended and pg_malloc_extended for frontend
and backend
This interface can be used to control at a lower level memory allocation
using an interface similar to MemoryContextAllocExtended, but this time
applied to CurrentMemoryContext on backend side, while on frontend side
a similar interface is available.
---
src/backend/utils/mmgr/mcxt.c| 37 ++
src/common/fe_memutils.c | 43 ++--
src/include/common/fe_memutils.h | 11 ++
src/include/utils/palloc.h | 1 +
4 files changed, 81 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index e2fbfd4..c42a6b6 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -864,6 +864,43 @@ palloc0(Size size)
return ret;
}
+void *
+palloc_extended(Size size, int flags)
+{
+ /* duplicates MemoryContextAllocExtended to avoid increased overhead */
+ void *ret;
+
+ AssertArg(MemoryContextIsValid(CurrentMemoryContext));
+ AssertNotInCriticalSection(CurrentMemoryContext);
+
+ if (((flags MCXT_ALLOC_HUGE) != 0 !AllocHugeSizeIsValid(size)) ||
+ ((flags MCXT_ALLOC_HUGE) == 0 !AllocSizeIsValid(size)))
+ elog(ERROR, invalid memory alloc request size %zu, size);
+
+ CurrentMemoryContext-isReset = false;
+
+ ret =
Re: [HACKERS] Supporting TAP tests with MSVC and Windows
Thanks for your input, Noah.
On Fri, Apr 3, 2015 at 3:22 PM, Noah Misch n...@leadboat.com wrote:
Each Windows patch should cover all Windows build systems; patches that fix a
problem in the src/tools/msvc build while leaving it broken in the GNU make
build are a bad trend. In this case, I expect you'll need few additional
changes to cover both.
Yes I am planning to do more tests with MinGW as well, once I got the
patch in a more advanced state in May/June. For Cygwin, I am not much
familiar with it yet, but I am sure I'll come up with something.
On Thu, Apr 02, 2015 at 06:30:02PM +0900, Michael Paquier wrote:
2) tapcheck does not check if IPC::Run is installed or not. Should we
add a check in the code for that? If yes, should we bypass the test or
fail?
The src/tools/msvc build system officially requires ActivePerl, and I seem to
recall ActivePerl installs IPC::Run by default. A check is optional.
I recall installing ActivePerl for my own box some time ago. It is
5.16, so now it is outdated, but the package manager does not contain
IPC::Run and I had to install it by hand.
Test suites that run as part of make check-world, including all src/bin TAP
suites, _must not_ enable trust authentication on Windows. To do so would
reintroduce CVE-2014-0067. (The standard alternative is to call pg_regress
--config-auth, which switches a data directory to SSPI authentication.)
Suites not in check-world, though not obligated to follow that rule, will have
a brighter future if they do so anyway.
OK. I'll rework this part.
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -73,9 +74,19 @@ sub tempdir_short
sub standard_initdb
{
my $pgdata = shift;
- system_or_bail(initdb -D '$pgdata' -A trust -N /dev/null);
- system_or_bail($ENV{top_builddir}/src/test/regress/pg_regress,
-'--config-auth', $pgdata);
+
+ if ($Config{osname} eq MSWin32)
+ {
+ system_or_bail(initdb -D $pgdata -A trust -N NUL);
+ system_or_bail($ENV{TESTREGRESS},
+'--config-auth', $pgdata);
+ }
+ else
+ {
+ system_or_bail(initdb -D '$pgdata' -A trust -N /dev/null);
+
system_or_bail($ENV{top_builddir}/src/test/regress/pg_regress,
+'--config-auth', $pgdata);
+ }
}
Make all build systems set TESTREGRESS, and use it unconditionally.
Yeah, that would be better.
That's not a complete review, just some highlights.
Thanks again.
--
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] Possibly a typo in expand_inherited_rtentry()
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
Re: [HACKERS] Auditing extension for PostgreSQL (Take 2)
On Thu, Apr 2, 2015 at 2:46 AM, David Steele da...@pgmasters.net 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) http://www.postgresql.org/message-id/20150325175954.gl3...@alvh.no-ip.org 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] Tables cannot have INSTEAD OF triggers
On 2 April 2015 at 22:23, Robert Haas robertmh...@gmail.com wrote: On Thu, Apr 2, 2015 at 5:02 PM, Andres Freund and...@anarazel.de wrote: I think the upshot is that INSTEAD OF triggers work in a particular way because that's what is needed to support updatable views. If triggers on tables should behave differently, maybe it should be a separate trigger type. Maybe it would be feasible to extend BEFORE triggers to support RETURNING, for example? What in the above prohibits extending the behaviour to tables? I have yet to see what compatibility or similarity problem that'd pose. It seems all mightily handwavy to me. Yeah. It's possible there's a better interface here than INSTEAD OF, and one of the things I didn't like about the OP was that it started by stating the syntax that would be used rather than by describing the problem that needed to be solved. It's generally better to start with the latter, and then work out the syntax from there. But having gotten that gripe out of my system, and especially in view of Dean's comments, it's not very clear to me what's wrong with using INSTEAD OF for this purpose. If you make BEFORE triggers do this via RETURNING, then you might have a trigger that returns multiple rows, which seems like it would introduce a bunch of new complexity for no obvious benefit. Yes, I'm inclined to agree. One of the reasons that INSTEAD OF triggers weren't supported on tables was the lack of an obvious use-case for it, but now having thought about partitioning, I think they would provide a fairly neat solution to that problem. I don't think that putting a view with INSTEAD OF triggers on top of the parent table and then always going through that view works quite as well, because there are still a few cases where a view doesn't work as well as a table. A view can't be the target of a foreign key, for example, so there'd be no way for a cascaded UPDATE to invoke the INSTEAD OF triggers. If you needed to handle the case of updates causing a change of partition, adding conditional INSTEAD OF triggers to the child tables would be a way to do that, retaining support for RETURNING, and keeping the logic localised to each partition, only invoking it when necessary. Supporting INSTEAD OF triggers on tables is not completely trivial to implement, but it doesn't look too hard either, and the more I think about it, the more I suspect that other use-cases will emerge to make that effort worthwhile. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Possibly a typo in expand_inherited_rtentry()
Hi, Attached does: 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); else Does that make sense? Thanks, Amit diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 51b3da2..b0cb818 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1317,7 +1317,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) 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); else -- 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
On 2015/03/25 4:56, Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp 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()
On Fri, Apr 3, 2015 at 2:30 PM, Michael Paquier michael.paqu...@gmail.com 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] Supporting TAP tests with MSVC and Windows
Each Windows patch should cover all Windows build systems; patches that fix a
problem in the src/tools/msvc build while leaving it broken in the GNU make
build are a bad trend. In this case, I expect you'll need few additional
changes to cover both.
On Thu, Apr 02, 2015 at 06:30:02PM +0900, Michael Paquier wrote:
2) tapcheck does not check if IPC::Run is installed or not. Should we
add a check in the code for that? If yes, should we bypass the test or
fail?
The src/tools/msvc build system officially requires ActivePerl, and I seem to
recall ActivePerl installs IPC::Run by default. A check is optional.
7) TAP tests use sometimes top_builddir directly. I think that's ugly,
and if there are no objections I think that we should change it to use
a dedicated environment variable like TESTTOP or similar.
I sense nothing ugly about a top_builddir reference, but see below.
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
open HBA, $tempdir/pgdata/pg_hba.conf;
-print HBA local replication all trust\n;
+# Windows builds do not support local connections
+if ($Config{osname} ne MSWin32)
+{
+ print HBA local replication all trust\n;
+}
print HBA host replication all 127.0.0.1/32 trust\n;
print HBA host replication all ::1/128 trust\n;
close HBA;
Test suites that run as part of make check-world, including all src/bin TAP
suites, _must not_ enable trust authentication on Windows. To do so would
reintroduce CVE-2014-0067. (The standard alternative is to call pg_regress
--config-auth, which switches a data directory to SSPI authentication.)
Suites not in check-world, though not obligated to follow that rule, will have
a brighter future if they do so anyway.
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -73,9 +74,19 @@ sub tempdir_short
sub standard_initdb
{
my $pgdata = shift;
- system_or_bail(initdb -D '$pgdata' -A trust -N /dev/null);
- system_or_bail($ENV{top_builddir}/src/test/regress/pg_regress,
-'--config-auth', $pgdata);
+
+ if ($Config{osname} eq MSWin32)
+ {
+ system_or_bail(initdb -D $pgdata -A trust -N NUL);
+ system_or_bail($ENV{TESTREGRESS},
+'--config-auth', $pgdata);
+ }
+ else
+ {
+ system_or_bail(initdb -D '$pgdata' -A trust -N /dev/null);
+ system_or_bail($ENV{top_builddir}/src/test/regress/pg_regress,
+'--config-auth', $pgdata);
+ }
}
Make all build systems set TESTREGRESS, and use it unconditionally.
That's not a complete review, just some highlights.
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
Re: [HACKERS] The return value of allocate_recordbuf()
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 mich...@otacoo.com
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
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 functions for them */
+
+
Re: [HACKERS] Abbreviated keys for text cost model fix
On Mon, Feb 23, 2015 at 7:44 PM, Peter Geoghegan p...@heroku.com wrote: On Mon, Feb 23, 2015 at 4:41 PM, Tomas Vondra tomas.von...@2ndquadrant.com 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] The return value of allocate_recordbuf()
On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier michael.paqu...@gmail.com 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] libpq's multi-threaded SSL callback handling is busted
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] Re: Abbreviated keys for Datum tuplesort
On Thu, Apr 2, 2015 at 7:02 PM, Peter Geoghegan p...@heroku.com wrote: On Thu, Apr 2, 2015 at 11:21 PM, Robert Haas robertmh...@gmail.com 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] Re: Abbreviated keys for Datum tuplesort
On Fri, Apr 3, 2015 at 1:17 PM, Stephen Frost sfr...@snowman.net 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] Auditing extension for PostgreSQL (Take 2)
On 4/3/15 3:59 AM, Sawada Masahiko wrote: On Thu, Apr 2, 2015 at 2:46 AM, David Steele da...@pgmasters.net 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) http://www.postgresql.org/message-id/20150325175954.gl3...@alvh.no-ip.org 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] Abbreviated keys for Numeric
On Thu, Apr 2, 2015 at 10:41 PM, Andrew Gierth and...@tao11.riddles.org.uk 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] Re: Abbreviated keys for Datum tuplesort
On Fri, Apr 3, 2015 at 1:07 PM, 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, 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 (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] Cube extension kNN support
On Thu, Mar 12, 2015 at 8:43 PM, Stas Kelvich stas.kelv...@gmail.com 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] Abbreviated keys for text cost model fix
On Fri, Apr 3, 2015 at 1:49 PM, Robert Haas robertmh...@gmail.com 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 == Robert Haas robertmh...@gmail.com 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] Table-level log_autovacuum_min_duration
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] initdb -S and tablespaces
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] Abbreviated keys for Numeric
Robert == Robert Haas robertmh...@gmail.com 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] Abbreviated keys for Numeric
On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth and...@tao11.riddles.org.uk 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
[HACKERS] Compile warnings on OSX 10.10 clang 6.0
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] Compile warnings on OSX 10.10 clang 6.0
Peter Eisentraut pete...@gmx.net 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] Compile warnings on OSX 10.10 clang 6.0
I wrote: Peter Eisentraut pete...@gmx.net 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] Table-level log_autovacuum_min_duration
On Fri, Apr 3, 2015 at 11:59 PM, Alvaro Herrera alvhe...@2ndquadrant.com 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] Abbreviated keys for Numeric
... 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] Compile warnings on OSX 10.10 clang 6.0
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
Re: [HACKERS] The return value of allocate_recordbuf()
On Fri, Apr 3, 2015 at 9:57 PM, Fujii Masao masao.fu...@gmail.com wrote: On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier michael.paqu...@gmail.com 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] Unused variable in hashpage.c
Petr Jelinek p...@2ndquadrant.com 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] Abbreviated keys for Numeric
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
Robert == Robert Haas robertmh...@gmail.com 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] Compile warnings on OSX 10.10 clang 6.0
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 t...@sss.pgh.pa.us wrote: I wrote: Peter Eisentraut pete...@gmx.net 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
Robert Haas robertmh...@gmail.com writes: On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth and...@tao11.riddles.org.uk 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 robertmh...@gmail.com 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
[HACKERS] COALESCE() query yield different result with MJ vs. NLJ/HJ
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] Abbreviated keys for Numeric
On Fri, Apr 3, 2015 at 5:45 PM, Tom Lane t...@sss.pgh.pa.us 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
Re: [HACKERS] Sloppy SSPI error reporting code
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] Providing catalog view to pg_hba.conf file - Patch submission
Hi 2015-03-31 14:38 GMT+02:00 Haribabu Kommi kommi.harib...@gmail.com: On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule pavel.steh...@gmail.com 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
On Fri, Apr 3, 2015 at 3:46 PM, Andrew Gierth and...@tao11.riddles.org.uk 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
On Fri, Apr 3, 2015 at 9:06 AM, Andrew Gierth and...@tao11.riddles.org.uk wrote: Robert == Robert Haas robertmh...@gmail.com 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] Possibly a typo in expand_inherited_rtentry()
Amit Langote langote_amit...@lab.ntt.co.jp 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] PATCH: adaptive ndistinct estimator v4
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] a fast bloat measurement tool (was Re: Measuring relation free space)
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 a...@2ndquadrant.com
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 a...@2ndquadrant.com
+ * 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
