Re: [HACKERS] Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Amit Langote]

On 06-01-2015 PM 04:26, Atri Sharma wrote:
> On Tue, Jan 6, 2015 at 12:43 PM, Amit Langote > wrote:
>> Though, I have no strong opinion on whether one thing is good or the
>> other or whether they cover some particular use case all the same.
>> Perhaps you can say that better.
>>
>>
> Personally, I think returning non ordered rows when ORDER BY clause is
> specifically specified by user is a gross violation of security and could
> lead to major user application breakdowns, since the application will trust
> that postgres will return the rows in order since ORDER BY was specified.
> Of course, what Ashutosh suggested makes the patch much simpler, but I
> would rather not go down that road.
> 

I think the same thing applies to IMMUTABLE declarations for example.
Planner trusts (or take as a hint) such declarations during, say,
constraint exclusion where quals involving non-immutable functions are
kept out of the exclusion proof. If a miscreant declares a non-immutable
function IMMUTABLE, then constraint violations may ensue simply because
planner trusted the miscreant. That is, such unsafe restrict clauses
would wrongly prove a partition as being unnecessary to scan. I am sure
there are other sites where such bets are made. In that light, I might
as well call them hints than anything.


The volatility category is a *promise* to the optimizer about the
behavior of the function


Though, as I said ordering behavior *may not be* a good candidate to
make such promises.

On the other hand, what such a thing might help with, are the situations
where a developer is frustrated because planner would ignore (or is
uninformed about) the order that the developer *knows* his function
produces.

But, if the node you propose to enforce the order is good enough, then
it may be worthwhile to go that route, :)

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

[HACKERS] Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Atri Sharma]

On Tuesday, January 6, 2015, Amit Langote 
wrote:

> On 06-01-2015 PM 04:26, Atri Sharma wrote:
> > On Tue, Jan 6, 2015 at 12:43 PM, Amit Langote <
> langote_amit...@lab.ntt.co.jp 
> >> wrote:
> >> Though, I have no strong opinion on whether one thing is good or the
> >> other or whether they cover some particular use case all the same.
> >> Perhaps you can say that better.
> >>
> >>
> > Personally, I think returning non ordered rows when ORDER BY clause is
> > specifically specified by user is a gross violation of security and could
> > lead to major user application breakdowns, since the application will
> trust
> > that postgres will return the rows in order since ORDER BY was specified.
> > Of course, what Ashutosh suggested makes the patch much simpler, but I
> > would rather not go down that road.
> >
>
> I think the same thing applies to IMMUTABLE declarations for example.
> Planner trusts (or take as a hint) such declarations during, say,
> constraint exclusion where quals involving non-immutable functions are
> kept out of the exclusion proof. If a miscreant declares a non-immutable
> function IMMUTABLE, then constraint violations may ensue simply because
> planner trusted the miscreant. That is, such unsafe restrict clauses
> would wrongly prove a partition as being unnecessary to scan. I am sure
> there are other sites where such bets are made. In that light, I might
> as well call them hints than anything.
>
> 
> The volatility category is a *promise* to the optimizer about the
> behavior of the function
> 
>
> Though, as I said ordering behavior *may not be* a good candidate to
> make such promises.
>
> On the other hand, what such a thing might help with, are the situations
> where a developer is frustrated because planner would ignore (or is
> uninformed about) the order that the developer *knows* his function
> produces.
>
> But, if the node you propose to enforce the order is good enough, then
> it may be worthwhile to go that route, :)
>
>
>
The purpose of the patch is to give the planner an option to use the
preorder that the developer knows will be produced. However, since ensuring
against developer induced errors in this case is relatively cheap, I think
the new node is worth it.


-- 
Regards,

Atri
*l'apprenant*

Re: [HACKERS] compiler warnings under MinGW for 9.4

[Michael Paquier]

On Tue, Dec 9, 2014 at 4:45 AM, Jeff Janes  wrote:
> In the past, building under MinGW produced so many warnings that I never
> bothered to read them.
>
> Now most of them have been removed, so the ones that are left might be worth
> reporting.
>
> Using gcc.exe (GCC) 4.6.2 on REL9_4_STABLE
> eadd80c08ddfc485db84b9af7cca54a0d50ebe6d I get:
>
> mingwcompat.c:60:1: warning: 'RegisterWaitForSingleObject' redeclared
> without dllimport attribute: previous dllimport ignored [-Wattributes]
> input.c:382:1: warning: 'saveHistory' defined but not used
> [-Wunused-function]
>
> Does anyone have opinions on how to address these?
Compiling with MinGW-32b, I am getting more of those than the ones you
mention, per se the attached. I'll try to come up with a patch to
reduce this amount on master.
-- 
Michael
configure: WARNING: *** Readline does not work on MinGW --- disabling
configure: WARNING: *** skipping thread test on Win32
dirmod.c: In function 'pgwin32_safestat':
dirmod.c:369:2: warning: implicit declaration of function 'stat' 
[-Wimplicit-function-declaration]
  r = stat(path, buf);
  ^
dirmod.c: In function 'pgwin32_safestat':
dirmod.c:369:2: warning: implicit declaration of function 'stat' 
[-Wimplicit-function-declaration]
  r = stat(path, buf);
  ^
twophase.c: In function 'ReadTwoPhaseFile':
twophase.c:1252:2: warning: passing argument 2 of '_fstat64i32' from 
incompatible pointer type [enabled by default]
  if (fstat(fd, &stat))
  ^
In file included from ../../../../src/include/port.h:283:0,
 from ../../../../src/include/c.h:1050,
 from ../../../../src/include/postgres.h:47,
 from twophase.c:36:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but 
argument is of type 'struct stat *'
 __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat) 
{
^
In file included from gram.y:14321:0:
scan.c: In function 'yy_try_NUL_trans':
scan.c:10188:23: warning: unused variable 'yyg' [-Wunused-variable]
 struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var may be 
unused depending upon options. */
   ^
copy.c: In function 'BeginCopyTo':
copy.c:1710:4: warning: passing argument 2 of '_fstat64i32' from incompatible 
pointer type [enabled by default]
fstat(fileno(cstate->copy_file), &st);
^
In file included from ../../../src/include/port.h:283:0,
 from ../../../src/include/c.h:1050,
 from ../../../src/include/postgres.h:47,
 from copy.c:15:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but 
argument is of type 'struct stat *'
 __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat) 
{
^
copy.c: In function 'BeginCopyFrom':
copy.c:2721:4: warning: passing argument 2 of '_fstat64i32' from incompatible 
pointer type [enabled by default]
fstat(fileno(cstate->copy_file), &st);
^
In file included from ../../../src/include/port.h:283:0,
 from ../../../src/include/c.h:1050,
 from ../../../src/include/postgres.h:47,
 from copy.c:15:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but 
argument is of type 'struct stat *'
 __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat) 
{
^
md5.c:88:0: warning: "G" redefined [enabled by default]
 #define G(x, y, z) (((x) & (z)) | ((y) & ~(z)))
 ^
:0:0: note: this is the location of the previous definition
basebackup.c: In function 'perform_base_backup':
basebackup.c:461:4: warning: passing argument 2 of '_fstat64i32' from 
incompatible pointer type [enabled by default]
if (fstat(fileno(fp), &statbuf) != 0)
^
In file included from ../../../src/include/port.h:283:0,
 from ../../../src/include/c.h:1050,
 from ../../../src/include/postgres.h:47,
 from basebackup.c:13:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but 
argument is of type 'struct stat *'
 __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat) 
{
^
snapmgr.c: In function 'ImportSnapshot':
snapmgr.c:1142:2: warning: passing argument 2 of '_fstat64i32' from 
incompatible pointer type [enabled by default]
  if (fstat(fileno(f), &stat_buf))
  ^
In file included from ../../../../src/include/port.h:283:0,
 from ../../../../src/include/c.h:1050,
 from ../../../../src/include/postgres.h:47,
 from snapmgr.c:42:
c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but 
argument is of type 'struct stat *'
 __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat) 
{
^
fe-connect.c: In function 'parseServiceInfo':
fe-connect.c:3899:3: warning: implicit declarat

Re: [HACKERS] What exactly is our CRC algorithm?

[Abhijit Menon-Sen]

At 2015-01-02 16:46:29 +0200, hlinnakan...@vmware.com wrote:
>
> In the slicing-by-8 version, I wonder if it would be better to do
> single-byte loads to c0-c7, instead of two 4-byte loads and shifts.

Nope. I did some tests, and the sb8 code is slightly slower if I remove
the 0-7byte alignment loop, and significantly slower if I switch to one
byte loads for the whole thing. So I think we should leave that part as
it is, but:

> Would it even make sense to keep the crc variable in different byte
> order, and only do the byte-swap once in END_CRC32() ?

…this certainly does make a noticeable difference. Will investigate.

> The comments need some work. I note that there is no mention of the
> slicing-by-8 algorithm anywhere in the comments (in the first patch).

Will fix. (Unfortunately the widely cited original Intel paper about
slice-by-8 seems to have gone AWOL, but I'll find something.)

> Instead of checking for "defined(__GNUC__) || defined(__clang__)",
> should add an explicit configure test for __builtin_bswap32().

Will do.

Thanks again.

-- Abhijit


-- 
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] SSL information view

[Magnus Hagander]

On Mon, Jan 5, 2015 at 9:56 PM, Peter Eisentraut  wrote:

> On 11/19/14 7:36 AM, Magnus Hagander wrote:
> > +  
> > +
>  
> pg_stat_sslpg_stat_ssl
> > +   One row per connection (regular and replication), showing
> statistics about
> > +SSL used on this connection.
> > +See  for details.
> > +   
> > +  
> > +
>
> It doesn't really show "statistics".  It shows information or data.
>

Good point.


We should make contrib/sslinfo a wrapper around this view as much as
> possible.
>

Agreed - but let's do that as a separate patch.


Is it useful to include rows for sessions not using SSL?
>

I think so, mainly because it makes things "more obvious" that you are
querying it the right way. Sure you could do a LEFT JOIN from
pg_stat_activity and a CASE to get the same results, but that makes it a
lot less in-your-face.


Should we perpetuate the "ssl"-naming?  Is there a more general term?
>

tls? :)

We call it ssl everywhere else, I think being consistent is important.


Will this work for non-OpenSSL implementations?
>

Yes, it uses (and declares new) the new internal APIs that Heikki created.



-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] tracking commit timestamps

[Petr Jelinek]

On 06/01/15 08:58, Michael Paquier wrote:

On Fri, Dec 19, 2014 at 3:53 PM, Noah Misch  wrote:

localhost template1=# select clock_timestamp(), pg_sleep(.1 * (n % 2)) from 
generate_series(0,7) t(n);
 clock_timestamp| pg_sleep
---+--
  2014-12-18 08:34:34.522126+00 |
  2014-12-18 08:34:34.522126+00 |
  2014-12-18 08:34:34.631508+00 |
  2014-12-18 08:34:34.631508+00 |
  2014-12-18 08:34:34.74089+00  |
  2014-12-18 08:34:34.74089+00  |
  2014-12-18 08:34:34.850272+00 |
  2014-12-18 08:34:34.850272+00 |
(8 rows)

So, we would need additional information other than the node ID *and*
the timestamp to ensure proper transaction commit ordering on Windows.
That's not cool and makes this feature very limited on this platform.



Well that's Windows time api for you, it affects everything that deals 
with timestamps though, not just commit ts. Note that the precision 
depends on hardware and other software that was running on the computer 
(there is undocumented api to increase the resolution, also use of 
multimedia timer increases resolution, etc).


The good news is that MS provides new high precision time API in Windows 
8 and Windows Server 2012 which we are using thanks to 
519b0757a37254452e013ea0ac95f4e56391608c so we are good at least on 
modern systems.


--
 Petr Jelinek  http://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] TABLESAMPLE patch

[Petr Jelinek]

On 06/01/15 08:51, Michael Paquier wrote:

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek  wrote:

Attached is v3 which besides the fixes mentioned above also includes changes
discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD), fixes for
crash with FETCH FIRST and is rebased against current master.

This patch needs a rebase, there is a small conflict in parallel_schedule.



Sigh, I really wish we had automation that checks this automatically for 
patches in CF.



Structurally speaking, I think that the tsm methods should be added in
src/backend/utils and not src/backend/access which is more high-level
as tsm_bernoulli.c and tsm_system.c contain only a set of new


I am not sure if I parsed this correctly, do you mean to say that only 
low-level access functions belong to src/backend/access? Makes sense.



procedure functions. Having a single header file tsm.h would be also a
good thing.


I was thinking about single header also, didn't find a good precedent so 
went with two, but don't have problem doing one :).



Regarding the naming, is "tsm" (table sample method) really appealing?
Wouldn't it be better to use simply tablesample_* for the file names
and the method names?



Doesn't really matter to me, I just really don't want to have 
tablesample_method_* there as that's way too long for my taste. I'll 
think about the naming when I move it to src/backend/utils.



This is a large patch... Wouldn't sampling.[c|h] extracted from
ANALYZE live better as a refactoring patch? This would limit a bit bug
occurrences on the main patch.



That's a good idea, I'll split it into patch series.

--
 Petr Jelinek  http://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

[HACKERS] Re: [COMMITTERS] pgsql: Change how first WAL segment on new timeline after promotion is

[Andres Freund]

On 2015-01-05 18:45:22 +0200, Heikki Linnakangas wrote:
> On 01/03/2015 08:59 PM, Andres Freund wrote:
> >Hi Heikki,
> >
> >While writing a test script for
> >http://archives.postgresql.org/message-id/20141205002854.GE21964%40awork2.anarazel.de
> >I noticed that this commit broke starting a pg_basebackup -X * without a
> >recovery.conf present. Which might not be the best idea, but imo is a
> >perfectly valid thing to do.
> >
> >To me the changes to StartupXLOG() in that commit look a bit bogus. The
> >new startLogSegNo is initialized to XLByteToSeg(EndOfLog)? Which points
> >to the end of the record +1? Which thus isn't guaranteed to exist as a
> >segment (e.g. never if the last record was a XLOG_SWITCH).
> 
> Ah, good point.
> 
> >Did you perhaps intend to use XLogFileInit(use_existing = true)
> >instead of XLogFileOpen()? That works for me.
> 
> Hmm, that doesn't sound right either. XLogFileInit is used when you switch
> to a new segment, not to open an old segment for writing. It happens to
> work, because with use_existing = true it will in fact always open the old
> segment, instead of creating a new one, but I don't think that's in the
> spirit of how that function's intended to be used.

Well, its docs say "Create a new XLOG file segment, or open a
pre-existing one.", so it's not that mismatched. We really don't know
whether the EndOfLog's segment already exist in this scenario.  Also,
doesn't XLogWrite() essentially use it in the same way?

> A very simple fix is to not try opening the segment at all. There isn't
> actually any requirement to have the segment open at that point, XLogWrite()
> will open it the first time the WAL is flushed. The WAL is flushed after
> writing the initial checkpoint or end-of-recovery record, which happens
> pretty soon anyway. Any objections to the attached?

Sounds like a good plan.

> >I've attached my preliminary testscript (note it's really not that
> >interesting at this point) that reliably reproduces the problem for me.
> 
> Thanks!

I've attached attached, for posterities sake, the last version of that
script. I think we should have at least some of these tests in the tap
tests. I'd not used that framework because I wanted to test back to 9.1,
but ...

> diff --git a/src/backend/access/transam/xlog.c 
> b/src/backend/access/transam/xlog.c
> index 5cc7e47..e623463 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -5646,7 +5646,6 @@ StartupXLOG(void)
>   XLogRecPtr  RecPtr,
>   checkPointLoc,
>   EndOfLog;
> - XLogSegNo   startLogSegNo;
>   TimeLineID  PrevTimeLineID;
>   XLogRecord *record;
>   TransactionId oldestActiveXID;
> @@ -6633,7 +6632,6 @@ StartupXLOG(void)
>*/
>   record = ReadRecord(xlogreader, LastRec, PANIC, false);
>   EndOfLog = EndRecPtr;
> - XLByteToSeg(EndOfLog, startLogSegNo);
>  
>   /*
>* Complain if we did not roll forward far enough to render the backup
> @@ -6741,9 +6739,6 @@ StartupXLOG(void)
>* buffer cache using the block containing the last record from the
>* previous incarnation.
>*/
> - openLogSegNo = startLogSegNo;
> - openLogFile = XLogFileOpen(openLogSegNo);
> - openLogOff = 0;
>   Insert = &XLogCtl->Insert;
>   Insert->PrevBytePos = XLogRecPtrToBytePos(LastRec);
>   Insert->CurrBytePos = XLogRecPtrToBytePos(EndOfLog);

The comment above could use some minor word smithing - the second part
of it doesn't really seem to belong there.

Greetings,

Andres Freund

-- 
 Andres Freund http://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] TABLESAMPLE patch

[Andres Freund]

On 2015-01-06 14:22:16 +0100, Petr Jelinek wrote:
> On 06/01/15 08:51, Michael Paquier wrote:
> >On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek  wrote:
> >>Attached is v3 which besides the fixes mentioned above also includes changes
> >>discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD), fixes for
> >>crash with FETCH FIRST and is rebased against current master.
> >This patch needs a rebase, there is a small conflict in parallel_schedule.
> >
> 
> Sigh, I really wish we had automation that checks this automatically for
> patches in CF.

FWIW, I personally think minor conflicts aren't really an issue and
don't really require a rebase. At least if the patches are in git
format, the reviewer can just use the version they're based on. Perhaps
always stating which version of the tree the patches apply on would be
good practice.

Greetings,

Andres Freund

-- 
 Andres Freund http://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] [RFC] Incremental backup v3: incremental PoC

[Robert Haas]

On Tue, Oct 14, 2014 at 1:17 PM, Marco Nenciarini
 wrote:
> I would to replace the getMaxLSN function with a more-or-less persistent
> structure which contains the maxLSN for each data segment.
>
> To make it work I would hook into the ForwardFsyncRequest() function in
> src/backend/postmaster/checkpointer.c and update an in memory hash every
> time a block is going to be fsynced. The structure could be persisted on
> disk at some time (probably on checkpoint).
>
> I think a good key for the hash would be a BufferTag with blocknum
> "rounded" to the start of the segment.
>
> I'm here asking for comments and advices on how to implement it in an
> acceptable way.

I'm afraid this is going to be quite tricky to implement.  There's no
way to make the in-memory hash table large enough that it can
definitely contain all of the entries for the entire database.  Even
if it's big enough at a certain point in time, somebody can create
100,000 new tables and now it's not big enough any more.  This is not
unlike the problem we had with the visibility map and free space map
before 8.4 (and you probably remember how much fun that was).

I suggest leaving this out altogether for the first version.  I can
think of three possible ways that we can determine which blocks need
to be backed up.  One, just read every block in the database and look
at the LSN of each one.  Two, maintain a cache of LSN information on a
per-segment (or smaller) basis, as you suggest here.  Three, scan the
WAL generated since the incremental backup and summarize it into a
list of blocks that need to be backed up.  This last idea could either
be done when the backup is requested, or it could be done as the WAL
is generated and used to populate the LSN cache.  In the long run, I
think some variant of approach #3 is likely best, but in the short
run, approach #1 (scan everything) is certainly easiest.  While it
doesn't optimize I/O, it still gives you the benefit of reducing the
amount of data that needs to be transferred and stored, and that's not
nothing.  If we get that much working, we can improve things more
later.

-- 
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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)

[Kouhei Kaigai]

Jim, Thanks for your reviewing the patch.

The attached patch is revised one according to your suggestion,
and also includes bug fix I could found.

* Definitions of TIDOperator was moved to pg_operator.h
  as other operator doing.
* Support the case of "ctid (operator) Param" expression.
* Add checks if commutator of TID was not found.
* Qualifiers gets extracted on plan stage, not path stage.
* Adjust cost estimation logic to fit SeqScan manner.
* Add some new test cases for regression test.

> On 12/24/14, 7:54 PM, Kouhei Kaigai wrote:
> >>> On Mon, Dec 15, 2014 at 4:55 PM, Kouhei Kaigai
> >>> 
> >> wrote:
>  I'm not certain whether we should have this functionality in
>  contrib from the perspective of workload that can help, but its
>  major worth is for an example of custom-scan interface.
> >>> worker_spi is now in src/test/modules. We may add it there as well,
> no?
> >>>
> >> Hmm, it makes sense for me. Does it also make sense to add a
> >> test-case to the core regression test cases?
> >>
> > The attached patch adds ctidscan module at test/module instead of contrib.
> > Basic portion is not changed from the previous post, but file
> > locations and test cases in regression test are changed.
> 
> First, I'm glad for this. It will be VERY valuable for anyone trying to
> clean up the end of a majorly bloated table.
> 
> Here's a partial review...
> 
> +++ b/src/test/modules/ctidscan/ctidscan.c
> 
> +/* missing declaration in pg_proc.h */
> +#ifndef TIDGreaterOperator
> +#define TIDGreaterOperator   2800
> ...
> If we're calling that out here, should we have a corresponding comment in
> pg_proc.h, in case these ever get renumbered?
> 
It was a quick hack when I moved this module out of the tree.
Yep, I should revert when I submit this patch again.

> +CTidQualFromExpr(Node *expr, int varno)
> ...
> + if (IsCTIDVar(arg1, varno))
> + other = arg2;
> + else if (IsCTIDVar(arg2, varno))
> + other = arg1;
> + else
> + return NULL;
> + if (exprType(other) != TIDOID)
> + return NULL;/* should not happen */
> + /* The other argument must be a pseudoconstant */
> + if (!is_pseudo_constant_clause(other))
> + return NULL;
> 
> I think this needs some additional blank lines...
> 
OK. And, I also noticed the coding style around this function is
different from other built-in plans, so I redefined the role of
this function just to check whether the supplied RestrictInfo is
OpExpr that involves TID inequality operator, or not.

Expression node shall be extracted when Plan node is created
from Path node.

> + if (IsCTIDVar(arg1, varno))
> + other = arg2;
> + else if (IsCTIDVar(arg2, varno))
> + other = arg1;
> + else
> + return NULL;
> +
> + if (exprType(other) != TIDOID)
> + return NULL;/* should not happen */
> +
> + /* The other argument must be a pseudoconstant */
> + if (!is_pseudo_constant_clause(other))
> + return NULL;
> 
> + * CTidEstimateCosts
> + *
> + * It estimates cost to scan the target relation according to the given
> + * restriction clauses. Its logic to scan relations are almost same as
> + * SeqScan doing, because it uses regular heap_getnext(), except for
> + * the number of tuples to be scanned if restriction clauses work well.
> 
> That should read "same as what SeqScan is doing"... however, what
> actual function are you talking about? I couldn't find SeqScanEstimateCosts
> (or anything ending EstimateCosts).
> 
It is cost_seqscan(). But I don't put a raw function name in the source
code comments in other portion, because nobody can guarantee it is up to
date in the future...

> BTW, there's other grammar issues but it'd be best to handle those all at
> once after all the code stuff is done.
> 
Yep. Help by native English speaker is very helpful for us.

> + opno = get_commutator(op->opno);
> What happens if there's no commutator? Perhaps best to Assert(opno !=
> InvalidOid) at the end of that if block.
> 
Usually, commutator operator of TID is defined on initdb time, however,
nobody can guarantee a mad superuser doesn't drop it.
So, I added elog(ERROR,...) here.


> Though, it seems things will fall apart anyway if ctid_quals isn't exactly
> what we're expecting; I don't know if that's OK or not.
> 
No worry, it was already checked on planning stage.

> + /* disk costs --- assume each tuple on a different page */
> + run_cost += spc_random_page_cost * ntuples;
> Isn't that extremely pessimistic?
> 
Probably. I follow the manner of SeqScan.

> I'm not familiar enough with the custom-scan stuff to really comment past
> this point, and I could certainly be missing some details about planning
> and ex

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Kouhei Kaigai]

Hello,

The attached patch is newer revision of custom-/foreign-join
interface.

I've ported my PG-Strom extension to fit the latest custom-scan
(+this patch) interface in this winter vacation.

The concept of "join replaced by foreign-/custom-scan" almost
works well, however, here are two small oversight on the v1
interface.

1. EXPLAIN didn't work when scanrelid==0.
ExplainNode() always called ExplainScanTarget() to T_ForeignScan
or T_CustomScan, however, foreign-/custom-scan node that replaced
join relation does not have a particular base relation.
So, I put a check to skip this call when scanrelid==0.

2. create_plan_recurse() needs to be available from extension.
In case when CustomScan node takes underlying plan nodes, its
PlanCustomPath() method is also responsible to invoke the plan
creation routine of the underlying path-node. However, existing
code declared create_plan_recurse() as static function.
So, this patch re-declared it as external function.


Also, one other point I'd like to have in this interface.
In case when foreign-/custom-scan node has pseudo-scan
targetlist, it may contain the target-entries which are not
actually in use, but need to be here to lookup column name on
EXPLAIN command.
I'd like to add a flag to indicate the core backend to ignore
target-entries in the pseudo-scan tlist if resjunk=true, when
it initialized the foreign-/custom-scan-state node, and setting
up scan type descriptor.
It will reduce unnecessary projection, if foreign-/custom-scan
node can produce a tuple based on the expectation of tlist.

I'd like to see the comment around this point.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei 


> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kouhei Kaigai
> Sent: Wednesday, December 03, 2014 3:11 PM
> To: Robert Haas
> Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada
> Subject: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
> 
> > On Tue, Nov 25, 2014 at 3:44 AM, Kouhei Kaigai 
> wrote:
> > > Today, I had a talk with Hanada-san to clarify which can be a common
> > > portion of them and how to implement it. Then, we concluded both of
> > > features can be shared most of the infrastructure.
> > > Let me put an introduction of join replacement by
> > > foreign-/custom-scan
> > below.
> > >
> > > Its overall design intends to inject foreign-/custom-scan node
> > > instead of the built-in join logic (based on the estimated cost).
> > > From the viewpoint of core backend, it looks like a sub-query scan
> > > that contains relations join internally.
> > >
> > > What we need to do is below:
> > >
> > > (1) Add a hook add_paths_to_joinrel() It gives extensions (including
> > > FDW drivers and custom-scan providers) chance to add alternative
> > > paths towards a particular join of relations, using ForeignScanPath
> > > or CustomScanPath, if it can run instead
> > of the built-in ones.
> > >
> > > (2) Informs the core backend varno/varattno mapping One thing we
> > > need to pay attention is, foreign-/custom-scan node that performs
> > > instead of the built-in join node must return mixture of values come
> > > from both relations. In case when FDW driver fetch a remote record
> > > (also, fetch a record computed by external computing resource), the
> > > most reasonable way is to store it on ecxt_scantuple of ExprContext,
> > > then kicks projection with varnode that references this slot.
> > > It needs an infrastructure that tracks relationship between original
> > > varnode and the alternative varno/varattno. We thought, it shall be
> > > mapped to INDEX_VAR and a virtual attribute number to reference
> > > ecxt_scantuple naturally, and this infrastructure is quite helpful
> > > for
> > both of ForegnScan/CustomScan.
> > > We'd like to add List *fdw_varmap/*custom_varmap variable to both of
> > > plan
> > nodes.
> > > It contains list of the original Var node that shall be mapped on
> > > the position according to the list index. (e.g, the first varnode is
> > > varno=INDEX_VAR and
> > > varattno=1)
> > >
> > > (3) Reverse mapping on EXPLAIN
> > > For EXPLAIN support, above varnode on the pseudo relation scan
> > > needed to be solved. All we need to do is initialization of
> > > dpns->inner_tlist on
> > > set_deparse_planstate() according to the above mapping.
> > >
> > > (4) case of scanrelid == 0
> > > To skip open/close (foreign) tables, we need to have a mark to
> > > introduce the backend not to initialize the scan node according to
> > > table definition, but according to the pseudo varnodes list.
> > > As earlier custom-scan patch doing, scanrelid == 0 is a
> > > straightforward mark to show the scan node is not combined with a
> > particular real relation.
> > > So, it also need to add special case handling around
> > > foreign-/custom-scan
> > code.
> > >
> > > We expect above changes are enough small to implement basic join
> > > pus

Re: [HACKERS] add modulo (%) operator to pgbench

[Robert Haas]

On Mon, Jan 5, 2015 at 10:37 AM, Fabien COELHO  wrote:
> Anyway, I suggest to keep that for another round and keep the Robert's
> isofunctional patch as it is before extending.

+1.  Let's please get the basic thing committed, and then people can
write more patches to extend and improve it.  There is no reason to
squash-merge every enhancement someone might want here into what I
wrote.

-- 
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] Updating copyright notices to 2015 for PGDG

[Bruce Momjian]

On Sat, Jan  3, 2015 at 01:45:37PM -0800, David Fetter wrote:
> On Sat, Jan 03, 2015 at 09:54:16PM +0900, Michael Paquier wrote:
> > Hi all,
> > 
> > Shouldn't we update the copyright notices to 2015 for PGDG like in
> > 7e04792? I mean those things mainly:
> > Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
> > Regards,
> 
> I just ran this:
> 
> ./src/tools/copyright.pl 
> Using current year:  2015
> Manually update doc/src/sgml/legal.sgml and 
> src/interfaces/libpq/libpq.rc.in too.
> Also update ./COPYRIGHT and doc/src/sgml/legal.sgml in all back branches.
> 
> and did what it said on the current branch.
> 
> Please find patch attached.

I will run the script today.  I didn't do it earlier because I want to
be current on reading community email before doing it.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
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] [RFC] Incremental backup v3: incremental PoC

[Jehan-Guillaume de Rorthais]

On Tue, 6 Jan 2015 08:26:22 -0500
Robert Haas  wrote:

> Three, scan the WAL generated since the incremental backup and summarize it
> into a list of blocks that need to be backed up.

This can be done from the archive side.  I was talking about some months ago
now:

  http://www.postgresql.org/message-id/51c4dd20.3000...@free.fr

One of the traps I could think of it that it requires "full_page_write=on" so
we can forge each block correctly. So collar is that we need to start a diff
backup right after a checkpoints then.

And even without "full_page_write=on", maybe we could add a function, say
"pg_start_backupdiff()", which would force to log full pages right after it
only, the same way "full_page_write" does after a checkpoint. Diff backups would
be possible from each LSN where we pg_start_backupdiff'ed till whenever.

Building this backup by merging versions of blocks from WAL is on big step.
But then, there is a file format to define, how to restore it and to decide what
tools/functions/GUCs to expose to admins.

After discussing with Magnus, he adviced me to wait for a diff backup file
format to emerge from online tools, like discussed here (by the time, that was
Michael's proposal based on pg_basebackup that was discussed). But I wonder how
easier it would be to do this the opposite way? If this idea of building diff
backup offline from archives is possible, wouldn't it remove a lot of trouble
you are discussing here?

Regards,
-- 
Jehan-Guillaume de Rorthais
Dalibo
http://www.dalibo.com


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

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

[Rahila Syed]

Hello,

>Yes, that's caused by ccb161b. Attached are rebased versions. 

Following are some comments, 

>uint16  hole_offset:15, /* number of bytes in "hole" */
Typo in description of hole_offset


>for (block_id = 0; block_id <= record->max_block_id; block_id++)
>-   {
>-   if (XLogRecHasBlockImage(record, block_id))
>-   fpi_len += BLCKSZ -
record->blocks[block_id].hole_length;
>-   }
>+   fpi_len += record->blocks[block_id].bkp_len;

IIUC, if condition, /if(XLogRecHasBlockImage(record, block_id))/ is
incorrectly removed from the above for loop.

>typedef struct XLogRecordCompressedBlockImageHeader
I am trying to understand the purpose behind declaration of the above
struct. IIUC, it is defined in order to introduce new field uint16 
raw_length and it has been declared as a separate struct from
XLogRecordBlockImageHeader to not affect the size of WAL record when
compression is off.
I wonder if it is ok to simply memcpy the uint16 raw_length in the
hdr_scratch when compression is on
and not have a separate header struct for it neither declare it in existing
header. raw_length can be a locally defined variable is XLogRecordAssemble
or it can be a field in registered_buffer struct like compressed_page.
I think this can simplify the code. 
Am I missing something obvious?   

> /*
>  * Fill in the remaining fields in the XLogRecordBlockImageHeader
>  * struct and add new entries in the record chain.
> */
   
>   bkpb.fork_flags |= BKPBLOCK_HAS_IMAGE;

This code line seems to be misplaced with respect to the above comment. 
Comment indicates filling of XLogRecordBlockImageHeader fields while
fork_flags is a field of XLogRecordBlockHeader.
Is it better to place the code close to following condition?
  if (needs_backup)
  {

>+  *the original length of the
>+ * block without its page hole being deducible from the compressed data
>+ * itself.
IIUC, this comment before XLogRecordBlockImageHeader seems to be no longer
valid as original length is not deducible from compressed data and rather
stored in header.


Thank you,
Rahila Syed



--
View this message in context: 
http://postgresql.nabble.com/Compression-of-full-page-writes-tp5769039p5833025.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Alvaro Herrera]

Tom Lane wrote:

> What would make sense to me is to teach the planner about inlining
> SQL functions that include ORDER BY clauses, so that the performance
> issue of a double sort could be avoided entirely transparently to
> the user.

Wouldn't this be applicable to functions in other languages too, not
only SQL?

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


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

Re: [HACKERS] Possible typo in create_policy.sgml

[Robert Haas]

On Tue, Jan 6, 2015 at 12:26 AM, Amit Langote
 wrote:
> Following is perhaps a typo:
>
> -   qualifications of queries which are run against the table the policy
> is on,
> +   qualifications of queries which are run against the table if the
> policy is on,
>
> Attached fixes it if so.

I don't think that's a typo, although it's not particularly
well-worded IMHO.  I might rewrite the whole paragraph like this:

A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
in a table to those rows which match the relevant policy expression.
Existing table rows are checked against the expression specified via
USING, while new rows that would be created via INSERT or UPDATE are
checked against the expression specified via WITH CHECK.  Generally,
the system will enforce filter conditions imposed using security
policies prior to qualifications that appear in the query itself, in
order to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy.  However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

-- 
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] INSERT ... ON CONFLICT UPDATE and RLS

[Robert Haas]

On Mon, Jan 5, 2015 at 12:54 PM, Peter Geoghegan  wrote:
> The patch that implements INSERT ... ON CONFLICT UPDATE has support
> and tests for per-column privileges (which are not relevant to the
> IGNORE variant, AFAICT). However, RLS support is another thing
> entirely. It has not been properly thought out, and unlike per-column
> privileges requires careful consideration, as the correct behavior
> isn't obvious.
>
> I've documented the current problems with RLS here:
>
> https://wiki.postgresql.org/wiki/UPSERT#RLS
>
> It's not clear whether or not the auxiliary UPDATE within an INSERT...
> ON CONFLICT UPDATE statement should have security quals appended.
> Stephen seemed to think that that might not be the best solution [1].
> I am not sure. I'd like to learn what other people think.
>
> What is the best way of integrating RLS with ON CONFLICT UPDATE? What
> behavior is most consistent with the guarantees of RLS? In particular,
> should the implementation append security quals to the auxiliary
> UPDATE, or fail sooner?

I think the INSERT .. ON CONFLICT UPDATE shouldn't be able to attempt
an update unless the UPDATE policies of the table are such that a
regular UPDATE would find the affected row.  The post-image of the row
needs to satisfy any UPDATE CHECK OPTION.  If the INSERT fails due to
a conflict with an unseen row, and the UPDATE can't find that row
either due to RLS, then it should probably error out; the alternative
is to silently do nothing, but that feels wrong.

-- 
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] Transactions involving multiple postgres foreign servers

[Robert Haas]

On Mon, Jan 5, 2015 at 3:23 PM, Tom Lane  wrote:
> Well, we intentionally didn't couple the FDW stuff closely into
> transaction commit, because of the thought that the "far end" would not
> necessarily have Postgres-like transactional behavior, and even if it did
> there would be about zero chance of having atomic commit with a
> non-Postgres remote server.  postgres_fdw is a seriously bad starting
> point as far as that goes, because it encourages one to make assumptions
> that can't possibly work for any other wrapper.

Atomic commit is something that can potentially be supported by many
different FDWs, as long as the thing on the other end supports 2PC.
If you're talking to Oracle or DB2 or SQL Server, and it supports 2PC,
then you can PREPARE the transaction and then go back and COMMIT the
transaction once it's committed locally.  Getting a cluster-wide
*snapshot* is probably a PostgreSQL-only thing requiring much deeper
integration, but I think it would be sensible to leave that as a
future project and solve the simpler problem first.

> I think the idea I sketched upthread of supporting an external transaction
> manager might be worth pursuing, in that it would potentially lead to
> having at least an approximation of atomic commit across heterogeneous
> servers.

An important threshold question here is whether we want to rely on an
external transaction manager, or build one into PostgreSQL.  As far as
this particular project goes, there's nothing that can't be done
inside PostgreSQL.  You need a durable registry of which transactions
you prepared on which servers, and which XIDs they correlate to.  If
you have that, then you can use background workers or similar to go
retry commits or rollbacks of prepared transactions until it works,
even if there's been a local crash meanwhile.

Alternatively, you could rely on an external transaction manager to do
all that stuff.  I don't have a clear sense of what that would entail,
or how it might be better or worse than rolling our own.  I suspect,
though, that it might amount to little more than adding a middle man.
I mean, a third-party transaction manager isn't going to automatically
know how to commit a transaction prepared on some foreign server using
some foreign data wrapper.  It's going to be have to be taught that if
postgres_fdw leaves a transaction in-medias-res on server OID 1234,
you've got to connect to the target machine using that foreign
server's connection parameters, speak libpq, and issue the appropriate
COMMIT TRANSACTION command.  And similarly, you're going to need to
arrange to notify it before preparing that transaction so that it
knows that it needs to request the COMMIT or ABORT later on.  Once
you've got all of that infrastructure for that in place, what are you
really gaining over just doing it in PostgreSQL (or, say, a contrib
module thereto)?

(I'm also concerned that an external transaction manager might need
the PostgreSQL client to be aware of it, whereas what we'd really like
here is for the client to just speak PostgreSQL and be happy that its
commits no longer end up half-done.)

-- 
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] psql -c does not honor ON_ERROR_STOP

[Joe Conway]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

This seems like either a psql bug or maybe just a documentation bug. I
know the psql docs say that -c behavior can be surprising, but I find
the below surprising even after reading the docs a couple of times.
Given that ON_ERROR_STOP defaults to off, it seems like these two
cases should both result in two rows inserted.

8<--
SQL="begin; insert into foo values(1); commit; begin; insert into foo
values(1); commit; begin; insert into foo values(2); commit;"

psql test -c "drop table if exists foo"
psql test -c "create table foo(id int primary key)"

psql test -c "${SQL}"
ERROR:  duplicate key value violates unique constraint "foo_pkey"
DETAIL:  Key (id)=(1) already exists.
psql test -c "select * from foo"
 id
- 
  1
(1 row)

psql test -c "truncate foo"
echo "${SQL}" | psql test
BEGIN
INSERT 0 1
COMMIT
BEGIN
ERROR:  duplicate key value violates unique constraint "foo_pkey"
DETAIL:  Key (id)=(1) already exists.
ROLLBACK
BEGIN
INSERT 0 1
COMMIT
psql test -c "select * from foo"
 id
- 
  1
  2
(2 rows)
8<--

Thoughts?

Joe

- -- 
Joseph E Conway
credativ LLC

270 E Douglas Ave
El Cajon, CA 92020

Office: +1 619 270 8787
Mobile: +1 619 843 8340

===
USA: http://www.credativ.us
Canada:  http://www.credativ.ca
Germany: http://www.credativ.de
Netherlands: http://www.credativ.nl
UK:  http://www.credativ.co.uk
India:   http://www.credativ.in
===
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQIcBAEBAgAGBQJUrClgAAoJEDfy90M199hld0IP/2mFzLZqN+wfK3gfBF6qe0W/
8Q1UYLC9n8uAPCBHWnhVuy+ypSjOqqmCrQCBwR4I1wpF0ni7gRwcTJ2pGYf3rh1o
HVCGqWfduDtRrdOjUaGVkSaNGdc73IjLQ+WFRAmfFAVpBpc3GkyWT3PG2I4+81IY
mDRxMgvf7n3NgzdKiCO7tci4dw4vvFOsmgCSA8opacYD/wPlx+IWGCr8uU6Ub9f7
S0ouKCTmfCGVzDjNs0HKkTtVrGmjqeS4c3dl56mFJf8Tuc0t9S2wSDUW2DmYWHu/
+zTLLo8XOzUnP6Uz1DXLzt4tBJSjNBgEgVx6OVe/MUdM/6+nrlI1VVHEC2EWZSEf
5OisX4kkIY1eqLj8ffTWUlCnyPFCb/5L3zVGFvxWL+FsenOxgTzrpGwEdPlOea1O
GYchUJlHT90CyTpqb/xHHpyhgy3u2raWTgD2lXgDJnzyUCrWSXXalgadrpyT6DnZ
JpIBW2vh00boOQOAy5yFwFFZ6r8XcUJzZEZbAdbsUY9f7BGq23mrrEW9QgEzj5VX
LEr8EKEK8VAa9VByBTl9Dch0MtcIP+8t9xYOhuF+winRujDpgrMIYiFjy0lhogWj
I885fWTU4sHWDOVOAxbVMILji+b6jALRuCQ6QcDq5Gx6ZIZeSZHfLGVNZO5LyxWR
gh8B3yMsshnNm3wCDrr/
=Te2e
-END PGP SIGNATURE-


-- 
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] psql -c does not honor ON_ERROR_STOP

[Tom Lane]

Joe Conway  writes:
> This seems like either a psql bug or maybe just a documentation bug. I
> know the psql docs say that -c behavior can be surprising, but I find
> the below surprising even after reading the docs a couple of times.
> Given that ON_ERROR_STOP defaults to off, it seems like these two
> cases should both result in two rows inserted.

-c submits the entire string to the backend in one PQexec(); therefore
ON_ERROR_STOP cannot have any impact on its behavior.  The backend will
abandon processing the whole string upon first error, embedded begin/
commit commands notwithstanding.

There's been repeated discussion of changing -c so that the string is
split apart and processed more like it would be if it'd been read from
stdin.  However, given the number of ways you can already submit a
string via stdin, this wouldn't be buying any new functionality.  Even
discounting backwards-compatibility considerations, such a change would
make it completely impossible to test multiple-commands-per-PQexec
scenarios using psql.  So I'm inclined to leave it alone.

Possibly the point is worth documenting explicitly, though.

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] Re: [COMMITTERS] pgsql: Change how first WAL segment on new timeline after promotion is

[Robert Haas]

On Tue, Jan 6, 2015 at 8:22 AM, Andres Freund  wrote:
> I've attached attached, for posterities sake, the last version of that
> script. I think we should have at least some of these tests in the tap
> tests. I'd not used that framework because I wanted to test back to 9.1,
> but ...

I don't see an attachment.

-- 
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: [COMMITTERS] pgsql: Change how first WAL segment on new timeline after promotion is

[Andres Freund]

On 2015-01-06 13:46:57 -0500, Robert Haas wrote:
> On Tue, Jan 6, 2015 at 8:22 AM, Andres Freund  wrote:
> > I've attached attached, for posterities sake, the last version of that
> > script. I think we should have at least some of these tests in the tap
> > tests. I'd not used that framework because I wanted to test back to 9.1,
> > but ...
> 
> I don't see an attachment.

Oops.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


basebackup_test.sh
Description: Bourne shell script

-- 
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] tracking commit timestamps

[Robert Haas]

On Tue, Jan 6, 2015 at 2:58 AM, Michael Paquier
 wrote:
> So, we would need additional information other than the node ID *and*
> the timestamp to ensure proper transaction commit ordering on Windows.
> That's not cool and makes this feature very limited on this platform.

You can't use the timestamp alone for commit ordering on any platform.
Eventually, two transactions will manage to commit in a single clock
tick, no matter how short that is.

Now, if we'd included the LSN in there...

-- 
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] XLOG_PARAMETER_CHANGE handling of wal_log_hints

[Petr Jelinek]

Hi,

when I was fixing how commit_ts handles the XLOG_PARAMETER_CHANGE I 
noticed that for wal_log_hints we assign the value in ControFile to 
current value instead of value that comes from WAL.


ISTM it has just information value so it should not have any practical 
impact, but it looks like a bug anyway so here is simple patch to change 
that.


--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5cc7e47..be67da4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8967,7 +8967,7 @@ xlog_redo(XLogReaderState *record)
 		ControlFile->max_prepared_xacts = xlrec.max_prepared_xacts;
 		ControlFile->max_locks_per_xact = xlrec.max_locks_per_xact;
 		ControlFile->wal_level = xlrec.wal_level;
-		ControlFile->wal_log_hints = wal_log_hints;
+		ControlFile->wal_log_hints = xlrec.wal_log_hints;
 		ControlFile->track_commit_timestamp = track_commit_timestamp;
 
 		/*

-- 
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] Possible typo in create_policy.sgml

[Stephen Frost]

Robert, Amit,

* Robert Haas (robertmh...@gmail.com) wrote:
> I don't think that's a typo, although it's not particularly
> well-worded IMHO.  I might rewrite the whole paragraph like this:
> 
> A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
> in a table to those rows which match the relevant policy expression.
> Existing table rows are checked against the expression specified via
> USING, while new rows that would be created via INSERT or UPDATE are
> checked against the expression specified via WITH CHECK.  Generally,
> the system will enforce filter conditions imposed using security
> policies prior to qualifications that appear in the query itself, in
> order to the prevent the inadvertent exposure of the protected data to
> user-defined functions which might not be trustworthy.  However,
> functions and operators marked by the system (or the system
> administrator) as LEAKPROOF may be evaluated before policy
> expressions, as they are assumed to be trustworthy.

Looks reasonable to me.  Amit, does this read better for you?  If so, I
can handle making the change to the docs.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] INSERT ... ON CONFLICT UPDATE and RLS

[Peter Geoghegan]

On Tue, Jan 6, 2015 at 9:39 AM, Robert Haas  wrote:
> I think the INSERT .. ON CONFLICT UPDATE shouldn't be able to attempt
> an update unless the UPDATE policies of the table are such that a
> regular UPDATE would find the affected row.  The post-image of the row
> needs to satisfy any UPDATE CHECK OPTION.  If the INSERT fails due to
> a conflict with an unseen row, and the UPDATE can't find that row
> either due to RLS, then it should probably error out; the alternative
> is to silently do nothing, but that feels wrong.

I can certainly see the argument for that behavior. I'm inclined to
agree that this is better.

With th existing implementation, UPDATE check options are effective at
preventing updates.  However, the implementation is not effective at
preventing row locking from finding a row that the user would not
otherwise be able to find (with a conventional UPDATE). So I guess
what I have to do now is figure out a way of also having the "... FOR
UPDATE   USING ( )" qual be enforced after row locking in respect
of the row locked (locked, but not yet used to generate a post-image)
in the target table. If it isn't satisfied, throw an error that
doesn't leak anything about the target row, rather than silently not
affecting the row. So, as you say, a divergence from what regular RLS
UPDATEs do that happens to make more sense here.

-- 
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] Updating copyright notices to 2015 for PGDG

[Stefan Kaltenbrunner]

On 01/06/2015 04:19 PM, Bruce Momjian wrote:
> On Sat, Jan  3, 2015 at 01:45:37PM -0800, David Fetter wrote:
>> On Sat, Jan 03, 2015 at 09:54:16PM +0900, Michael Paquier wrote:
>>> Hi all,
>>>
>>> Shouldn't we update the copyright notices to 2015 for PGDG like in
>>> 7e04792? I mean those things mainly:
>>> Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
>>> Regards,
>>
>> I just ran this:
>>
>> ./src/tools/copyright.pl 
>> Using current year:  2015
>> Manually update doc/src/sgml/legal.sgml and 
>> src/interfaces/libpq/libpq.rc.in too.
>> Also update ./COPYRIGHT and doc/src/sgml/legal.sgml in all back branches.
>>
>> and did what it said on the current branch.
>>
>> Please find patch attached.
> 
> I will run the script today.  I didn't do it earlier because I want to
> be current on reading community email before doing it.

hmm is it intentional that the commit also changed other files?

looks like the commited patch added newlines to various files that had
none before for example:

src/test/isolation/specs/nowait-2.spec
src/test/isolation/specs/nowait-3.spec
src/test/isolation/specs/skip-locked-4.spec
src/test/modules/commit_ts/commit_ts.conf


http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=4baaf863eca5412e07a8441b3b7e7482b7a8b21a#patch1352


while I do think that the files should have newlines I dont think those
should be added in a copyright bump commit and I think the script might
actually break files where we specifically dont want a newline (afaik we
dont have atm but still)


Stefan


-- 
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] Possible typo in create_policy.sgml

[Peter Geoghegan]

On Tue, Jan 6, 2015 at 11:25 AM, Stephen Frost  wrote:
> Looks reasonable to me.  Amit, does this read better for you?  If so, I
> can handle making the change to the docs.

The docs also prominently say:

"The security-barrier qualifications will always be evaluated prior to
any user-defined functions or user-provided WHERE clauses, while the
with-check expression will be evaluated against the rows which are
going to be added to the table. By adding policies to a table, a user
can limit the rows which a given user can select, insert, update, or
delete. This capability is also known as Row Level Security or RLS."

I would prefer it if it was clearer based on the syntax description
which qual is which. The security barrier qual "expression" should
have an identifier/name in the syntax description that is more
suggestive of "security barrier qual", emphasizing its distinctness
from "check_expression". For example, I think "barrier_expression"
would be clearer.
-- 
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] parallel mode and parallel contexts

[Simon Riggs]

On 6 January 2015 at 16:33, Robert Haas  wrote:

>> These comments don’t have any explanation or justification
>
> OK, I rewrote them.  Hopefully it's better now.

Thanks for new version and answers.

>> * Transaction stuff strikes me as overcomplicated and error prone.
>> Given there is no explanation or justification for most of it, I’d be
>> inclined to question why its there
>
> Gosh, I was pretty pleased with how simple the transaction integration
> turned out to be.  Most of what's there right now is either (a) code
> to copy state from the master to the parallel workers or (b) code to
> throw errors if the workers try to things that aren't safe.  I suspect
> there are a few things missing, but I don't see anything there that
> looks unnecessary.

If you can explain it in more detail in comments and README, I may
agree. At present, I don't get it and it makes me nervous.

The comment says
"Only the top frame of the transaction state stack is copied to a parallel
worker"
but I'm not sure why.

Top meaning the current subxact or the main xact?
If main, why are do we need XactTopTransactionId


>> This is very impressive, but it looks like we’re trying to lift too
>> much weight on the first lift. If we want to go anywhere near this, we
>> need to have very clear documentation about how and why its like that.
>> I’m actually very sorry to say that because the review started very
>> well, much much better than most.
>
> When I posted the group locking patch, it got criticized because it
> didn't actually do anything useful by itself; similarly, the
> pg_background patch was criticized for not being a large enough step
> toward parallelism.  So, this time, I posted something more
> comprehensive.  I don't think it's quite complete yet.  I expect a
> committable version of this patch to be maybe another 500-1000 lines
> over what I have here right now -- I think it needs to do something
> about heavyweight locking, and I expect that there are some unsafe
> things that aren't quite prohibited yet.  But the current patch is
> only 2300 lines, which is not astonishingly large for a feature of
> this magnitude; if anything, I'd say it's surprisingly small, due to a
> year and a half of effort laying the necessary groundwork via long
> series of preliminary commits.  I'm not unwilling to divide this up
> some more if we can agree on a way to do that that makes sense, but I
> think we're nearing the point where we need to take the plunge and
> say, look, this is version one of parallelism.  Thunk.

I want this also; the only debate is where to draw the line and please
don't see that as criticism.

I'm very happy it's so short, I agree it could be longer.

-- 
 Simon Riggs   http://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] pg_rewind in contrib

[Peter Eisentraut]

I applaud the ingenuity on all levels in this patch.  But it seems to me
that there is way too much backend knowledge encoded and/or duplicated
in a front-end program.

If this ends up shipping, it's going to be a massively popular tool.  I
see it as a companion to pg_basebackup.  So it should sort of work the
same way.  One problem is that it doesn't use the replication protocol,
so the setup is going to be inconsistent with pg_basebackup.  Maybe the
replication protocol could be extended to provide the required data.
Maybe something as simple as "give me this file" would work.

That might lose the local copy mode, but how important is that?
pg_basebackup doesn't have that mode.  In any case, the documentation
doesn't explain this distinction.  The option documentation is a bit
short in any case, but it's not clear that you can choose between local
and remote mode.

The test suite should probably be reimplemented in Perl.  (I might be
able to help.)  Again, ingenious, but it's very hard to follow the
sequence of what is being tested.  And some Windows person is going to
complain. ;-)

Also, since you have been maintaining this tool for a while, what is the
effort for maintaining it from version to version?


-- 
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] INSERT ... ON CONFLICT UPDATE and RLS

[Peter Geoghegan]

Another issue that I see is that there is only one resultRelation
between the INSERT and UPDATE. That means that without some additional
special measure, both UPDATE and INSERT "WITH CHECK ( ... ) " options
are applied regardless of whether the INSERT path was taken, or the
UPDATE path. Maybe that's actually sensible (note that even this
doesn't happen right now, since the auxiliary UPDATE is currently
minimally processed by the rewriter). What do people think about that?
It seems like it might be less surprising to have that fail earlier
when there are separate policies for INSERT and UPDATE -- for UPSERT,
all policies are applied regardless of which path is taken.

The task of making the security qual enforced like a check option
before we go to update a locked row (at the start of the UPDATE path,
for any UPDATE policy with a USING security qual) looks complicated.
I'd appreciate a little help on the implementation details.

Apparently Oracle does not offer "fine-grained access control" with
MERGE, which I believe means they just don't support this kind of
thing at all. Obviously I'd rather avoid that here, but the correct
semantics are not obvious. ON CONFLICT UPDATE could almost justify
making CREATE POLICY FOR INSERT accept a USING expression, since
that's really where the row to UPDATE comes from.
-- 
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] Updating copyright notices to 2015 for PGDG

[Bruce Momjian]

On Tue, Jan  6, 2015 at 08:46:19PM +0100, Stefan Kaltenbrunner wrote:
> > I will run the script today.  I didn't do it earlier because I want to
> > be current on reading community email before doing it.
> 
> hmm is it intentional that the commit also changed other files?
> 
> looks like the commited patch added newlines to various files that had
> none before for example:

Specifically, these files had no newline after the last line in the
file.

> src/test/isolation/specs/nowait-2.spec
> src/test/isolation/specs/nowait-3.spec
> src/test/isolation/specs/skip-locked-4.spec
> src/test/modules/commit_ts/commit_ts.conf
> 
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=4baaf863eca5412e07a8441b3b7e7482b7a8b21a#patch1352
> 
> while I do think that the files should have newlines I dont think those
> should be added in a copyright bump commit and I think the script might
> actually break files where we specifically dont want a newline (afaik we
> dont have atm but still)

Well, I am guessing the Perl 'tie' is adding them as there is no
explicit newline added in the script, and the Tie docs confirm that:

http://search.cpan.org/~toddr/Tie-File-1.00/lib/Tie/File.pm

Because the chomped value will have the separator reattached when it is
written back to the file. There is no way to create a file whose
trailing record separator string is missing.

There are probably other scripts that assume all lines end in a newline.
Is it worth changing the copyright script to preserve the lack of
newlines --- I doubt it.  I have added a Perl comment about this
behavior, though.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
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] parallel mode and parallel contexts

[Robert Haas]

On Tue, Jan 6, 2015 at 3:04 PM, Simon Riggs  wrote:
> If you can explain it in more detail in comments and README, I may
> agree. At present, I don't get it and it makes me nervous.
>
> The comment says
> "Only the top frame of the transaction state stack is copied to a parallel
> worker"
> but I'm not sure why.
>
> Top meaning the current subxact or the main xact?
> If main, why are do we need XactTopTransactionId

Current subxact.

I initially thought of copying the entire TransactionStateData stack,
but Heikki suggested (via IM) that I do it this way instead.  I
believe his concern was that it's never valid to commit or roll back
to a subtransaction that is not at the top of the stack, and if you
don't copy the stack, you avoid the risk of somehow ending up in that
state.  Also, you avoid having to invent resource owners for
(sub)transactions that don't really exist in the current process.  On
the other hand, you do end up with a few special cases that wouldn't
exist with the other approach.  Still, I'm pretty happy to have taken
Heikki's advice: it was certainly simple to implement this way, plus
hopefully that way at least one person likes what I ended up with.
:-)

What else needs clarification?

-- 
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] Possible typo in create_policy.sgml

[Robert Haas]

On Tue, Jan 6, 2015 at 2:48 PM, Peter Geoghegan  wrote:
> On Tue, Jan 6, 2015 at 11:25 AM, Stephen Frost  wrote:
>> Looks reasonable to me.  Amit, does this read better for you?  If so, I
>> can handle making the change to the docs.
>
> The docs also prominently say:
>
> "The security-barrier qualifications will always be evaluated prior to
> any user-defined functions or user-provided WHERE clauses, while the
> with-check expression will be evaluated against the rows which are
> going to be added to the table. By adding policies to a table, a user
> can limit the rows which a given user can select, insert, update, or
> delete. This capability is also known as Row Level Security or RLS."
>
> I would prefer it if it was clearer based on the syntax description
> which qual is which. The security barrier qual "expression" should
> have an identifier/name in the syntax description that is more
> suggestive of "security barrier qual", emphasizing its distinctness
> from "check_expression". For example, I think "barrier_expression"
> would be clearer.

I thought my rewrite clarified this distinction pretty well.  Maybe
I'm wrong?  We're talking about the same paragraph.

-- 
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] Possible typo in create_policy.sgml

[Peter Geoghegan]

On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas  wrote:
> I thought my rewrite clarified this distinction pretty well.  Maybe
> I'm wrong?  We're talking about the same paragraph.


Sorry, I didn't express myself clearly. I think that you did get it
right, but I would like to see that distinction also reflected in the
actual sgml PARAMETER class tag. "expression" is way too generic here.

-- 
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] Updating copyright notices to 2015 for PGDG

[Alvaro Herrera]

Bruce Momjian wrote:
> On Tue, Jan  6, 2015 at 08:46:19PM +0100, Stefan Kaltenbrunner wrote:
> > > I will run the script today.  I didn't do it earlier because I want to
> > > be current on reading community email before doing it.
> > 
> > hmm is it intentional that the commit also changed other files?
> > 
> > looks like the commited patch added newlines to various files that had
> > none before for example:
> 
> Specifically, these files had no newline after the last line in the
> file.

I don't think we have any files that require not to have a trailing
newline.  Do we need an explicit check against it?  Seems doubtful, but
then if the need arises, we will break it each year and who knows if
anybody will be vigilant enough to notice.  Stefan caught it this time,
but who would normally skim 18000 lines of supposedly mechanical diff
looking for issues?  (How did you catch this in the first place?)

This makes me wonder however how wise it is to update the copyright
notices in every single file in the repo.  Why do we need this?  Why not
abolish the practice and live forever with most files having copyright
2015?  (Only new files would have newer years in their copyright
notices, I guess.)  Does this provide us with any kind of protection,
and if so against what, and how does it protect us?  Since we have a
very clean git history which shows us the exact provenance of every
single line of source code, and we have excellent mail archives that
show where each line came from for all development in the last decade,
this single line of (C) boilerplate in each file seems completely
pointless.

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


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

[HACKERS] VODKA?

[Josh Berkus]

Oleg, Teodor:

I take it VODKA is sliding to version 9.6?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Updating copyright notices to 2015 for PGDG

[Bruce Momjian]

On Tue, Jan  6, 2015 at 06:12:30PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Tue, Jan  6, 2015 at 08:46:19PM +0100, Stefan Kaltenbrunner wrote:
> > > > I will run the script today.  I didn't do it earlier because I want to
> > > > be current on reading community email before doing it.
> > > 
> > > hmm is it intentional that the commit also changed other files?
> > > 
> > > looks like the commited patch added newlines to various files that had
> > > none before for example:
> > 
> > Specifically, these files had no newline after the last line in the
> > file.
> 
> I don't think we have any files that require not to have a trailing
> newline.  Do we need an explicit check against it?  Seems doubtful, but
> then if the need arises, we will break it each year and who knows if
> anybody will be vigilant enough to notice.  Stefan caught it this time,
> but who would normally skim 18000 lines of supposedly mechanical diff
> looking for issues?  (How did you catch this in the first place?)

I am guessing pgindent would also add a newline, but since the spec
files aren't C files, pgindent doesn't tough them.

> This makes me wonder however how wise it is to update the copyright
> notices in every single file in the repo.  Why do we need this?  Why not
> abolish the practice and live forever with most files having copyright
> 2015?  (Only new files would have newer years in their copyright
> notices, I guess.)  Does this provide us with any kind of protection,
> and if so against what, and how does it protect us?  Since we have a
> very clean git history which shows us the exact provenance of every
> single line of source code, and we have excellent mail archives that
> show where each line came from for all development in the last decade,
> this single line of (C) boilerplate in each file seems completely
> pointless.

I think the copyright update is more of a consistency thing, rather than
something that has a legal requirement.  It is hard to see why we would
stop doing it just to preserve missing trailing newlines we don't even
know we need.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
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] Turning recovery.conf into GUCs

[Peter Eisentraut]

On 1/6/15 12:57 AM, Josh Berkus wrote:
> On 01/05/2015 05:43 PM, Peter Eisentraut wrote:
>> The wins on the other hand are obscure: You can now use SHOW to inspect
>> recovery settings.  You can design your own configuration file include
>> structures to set them.  These are not bad, but is that all?
> 
> That's not the only potential win, and it's not small either. I'll be
> able to tell what master a replica is replicating from using via a port
> 5432 connection (currently there is absolutely no way to do this).

That's one particular case of what I mentioned above under using SHOW to
inspect recovery settings.  I agree that that's important, but it
doesn't look like there is a consensus that it justifies all the drawbacks.

That said, there is a much simpler way to achieve that specific
functionality: Expose all the recovery settings as fake read-only GUC
variables.  See attached patch for an example.

Btw., I'm not sure that everyone will be happy to have primary_conninfo
visible, since it might contain passwords.

> ... and there you hit on one of the other issues with recovery.conf,
> which is that it's a configuration file with configuration parameters
> which gets automatically renamed when a standby is promoted.  This plays
> merry hell with configuration management systems.  The amount of
> conditional logic I've had to write for Salt to handle recovery.conf
> truly doesn't bear thinking about.  There may be some other way to make
> recovery.conf configuration-management friendly, but I haven't thought
> of it.

I have written similar logic, and while it's not pleasant, it's doable.
 This issue would really only go away if you don't use a file to signal
recovery at all, which you have argued for, but which is really a
separate and more difficult problem.

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 218de87..477b906 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -237,10 +237,10 @@ static int	recovery_min_apply_delay = 0;
 static TimestampTz recoveryDelayUntilTime;
 
 /* options taken from recovery.conf for XLOG streaming */
-static bool StandbyModeRequested = false;
-static char *PrimaryConnInfo = NULL;
-static char *PrimarySlotName = NULL;
-static char *TriggerFile = NULL;
+bool StandbyModeRequested = false;
+char *PrimaryConnInfo = NULL;
+char *PrimarySlotName = NULL;
+char *TriggerFile = NULL;
 
 /* are we currently in standby mode? */
 bool		StandbyMode = false;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f6df077..6caadff 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -864,6 +864,15 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
+		{"standby_mode", PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop("TODO"),
+			NULL
+		},
+		&StandbyModeRequested,
+		false,
+		NULL, NULL, NULL
+	},
+	{
 		{"fsync", PGC_SIGHUP, WAL_SETTINGS,
 			gettext_noop("Forces synchronization of updates to disk."),
 			gettext_noop("The server will use the fsync() system call in several places to make "
@@ -3275,6 +3284,37 @@ static struct config_string ConfigureNamesString[] =
 	},
 
 	{
+		{"primary_conninfo", PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop("TODO"),
+			NULL,
+			GUC_SUPERUSER_ONLY
+		},
+		&PrimaryConnInfo,
+		NULL,
+		NULL, NULL, NULL
+	},
+
+	{
+		{"primary_slot_name", PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop("TODO"),
+			NULL
+		},
+		&PrimarySlotName,
+		NULL,
+		NULL, NULL, NULL
+	},
+
+	{
+		{"trigger_file", PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop("TODO"),
+			NULL
+		},
+		&TriggerFile,
+		NULL,
+		NULL, NULL, NULL
+	},
+
+	{
 		{"application_name", PGC_USERSET, LOGGING_WHAT,
 			gettext_noop("Sets the application name to be reported in statistics and logs."),
 			NULL,
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 138deaf..eeb9461 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -100,6 +100,11 @@ extern bool fullPageWrites;
 extern bool wal_log_hints;
 extern bool log_checkpoints;
 
+extern bool StandbyModeRequested;
+extern char *PrimaryConnInfo;
+extern char *PrimarySlotName;
+extern char *TriggerFile;
+
 /* WAL levels */
 typedef enum WalLevel
 {

-- 
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] Updating copyright notices to 2015 for PGDG

[Stefan Kaltenbrunner]

On 01/06/2015 10:12 PM, Alvaro Herrera wrote:
> Bruce Momjian wrote:
>> On Tue, Jan  6, 2015 at 08:46:19PM +0100, Stefan Kaltenbrunner wrote:
 I will run the script today.  I didn't do it earlier because I want to
 be current on reading community email before doing it.
>>>
>>> hmm is it intentional that the commit also changed other files?
>>>
>>> looks like the commited patch added newlines to various files that had
>>> none before for example:
>>
>> Specifically, these files had no newline after the last line in the
>> file.
> 
> I don't think we have any files that require not to have a trailing
> newline.  Do we need an explicit check against it?  Seems doubtful, but
> then if the need arises, we will break it each year and who knows if
> anybody will be vigilant enough to notice.  Stefan caught it this time,
> but who would normally skim 18000 lines of supposedly mechanical diff
> looking for issues?  (How did you catch this in the first place?)

yeah while the trailing newline thingy does not seem to be a real issue
it still caught my eye when I was glancing at the diff (I was basically
scrolling through it when I noticed this)

> 
> This makes me wonder however how wise it is to update the copyright
> notices in every single file in the repo.  Why do we need this?  Why not
> abolish the practice and live forever with most files having copyright
> 2015?  (Only new files would have newer years in their copyright
> notices, I guess.)  Does this provide us with any kind of protection,
> and if so against what, and how does it protect us?  Since we have a
> very clean git history which shows us the exact provenance of every
> single line of source code, and we have excellent mail archives that
> show where each line came from for all development in the last decade,
> this single line of (C) boilerplate in each file seems completely
> pointless.

I dont know why it is really needed but maybe for the files that have
identical copyrights one could simple reference to the COPYRIGHT file we
already have in the tree?


Stefan


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

Re: [HACKERS] pg_basebackup fails with long tablespace paths

[Peter Eisentraut]

On 12/27/14 8:02 PM, Robert Haas wrote:
> On Wed, Dec 24, 2014 at 8:12 AM, Peter Eisentraut  wrote:
>> On 12/22/14 10:00 PM, Amit Kapila wrote:
>>> There is already a patch [1] in this CF which will handle both cases, so
>>> I am
>>> not sure if it is very good idea to go with a new tar format to handle this
>>> issue.
>>
>> I think it would still make sense to have proper symlinks in the
>> basebackup if possible, for clarity.
> 
> I guess I would have assumed it would be more clear to omit the
> symlinks if we're expecting the server to put them in.  Otherwise, the
> server has to remove the existing symlinks and create new ones, which
> introduces various possibilities for failure and confusion.

Currently, when you unpack a tarred basebackup with tablespaces, the
symlinks will tell you whether you have unpacked the tablespace tars at
the right place.  Otherwise, how do you know?  Secondly, you also have
the option of putting the tablespaces somewhere else by changing the
symlinks.  Under the new scheme, the existing symlinks would be
overwritten (or not?).  If that is actually correct, then the proposed
fix doesn't really replicate the required functionality on Windows.

One way to address this would be to do away with the symlinks altogether
and have pg_tblspc/12345 be a text file that contains the tablespace
location.  Kind of symlinks implemented in user space.



-- 
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] parallel mode and parallel contexts

[Simon Riggs]

On 6 January 2015 at 21:01, Robert Haas  wrote:
> On Tue, Jan 6, 2015 at 3:04 PM, Simon Riggs  wrote:
>> If you can explain it in more detail in comments and README, I may
>> agree. At present, I don't get it and it makes me nervous.
>>
>> The comment says
>> "Only the top frame of the transaction state stack is copied to a parallel
>> worker"
>> but I'm not sure why.
>>
>> Top meaning the current subxact or the main xact?
>> If main, why are do we need XactTopTransactionId
>
> Current subxact.

TopTransactionStateData points to the top-level transaction data, but
XactTopTransactionId points to the current subxact.

So when you say "Only the top frame of the transaction state stack is
copied" you don't mean the top, you mean the bottom (the latest
subxact)? Which then becomes the top in the parallel worker? OK...

> I initially thought of copying the entire TransactionStateData stack,
> but Heikki suggested (via IM) that I do it this way instead.  I
> believe his concern was that it's never valid to commit or roll back
> to a subtransaction that is not at the top of the stack, and if you
> don't copy the stack, you avoid the risk of somehow ending up in that
> state.  Also, you avoid having to invent resource owners for
> (sub)transactions that don't really exist in the current process.  On
> the other hand, you do end up with a few special cases that wouldn't
> exist with the other approach.  Still, I'm pretty happy to have taken
> Heikki's advice: it was certainly simple to implement this way, plus
> hopefully that way at least one person likes what I ended up with.
> :-)
>
> What else needs clarification?

Those comments really belong in a README, not the first visible
comment in xact.c

You need to start with the explanation that parallel workers have a
faked-up xact stack to make it easier to copy and manage. That is
valid because we never change xact state during a worker operation.

I get it now and agree, but please work some more on clarity of
README, comments and variable naming!

-- 
 Simon Riggs   http://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] Turning recovery.conf into GUCs

[Josh Berkus]

On 01/06/2015 01:22 PM, Peter Eisentraut wrote:

> That said, there is a much simpler way to achieve that specific
> functionality: Expose all the recovery settings as fake read-only GUC
> variables.  See attached patch for an example.

I have no objections to that idea in principle.  As long as it gets into
9.5.

> Btw., I'm not sure that everyone will be happy to have primary_conninfo
> visible, since it might contain passwords.

Didn't we discuss this?  I forgot what the conclusion was ... probably
not to put passwords in primary_conninfo.

> 
>> ... and there you hit on one of the other issues with recovery.conf,
>> which is that it's a configuration file with configuration parameters
>> which gets automatically renamed when a standby is promoted.  This plays
>> merry hell with configuration management systems.  The amount of
>> conditional logic I've had to write for Salt to handle recovery.conf
>> truly doesn't bear thinking about.  There may be some other way to make
>> recovery.conf configuration-management friendly, but I haven't thought
>> of it.
> 
> I have written similar logic, and while it's not pleasant, it's doable.
>  This issue would really only go away if you don't use a file to signal
> recovery at all, which you have argued for, but which is really a
> separate and more difficult problem.

As far as CMSes are concerned, there is a vast difference between a file
which contains configuration variables and one which does not.  That is,
an *empty* recovery.conf file which just signals the start of recovery
is not a configuration problem.  The problem comes in in that
recovery.conf serves two separate purposes: it's a configuration file,
and it's also a trigger file.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Jim Nasby]

On 1/6/15, 8:17 AM, Kouhei Kaigai wrote:

The attached patch is newer revision of custom-/foreign-join
interface.


Shouldn't instances of

scan_relid > 0

be

scan_relid != InvalidOid

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


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

Re: [HACKERS] Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Jim Nasby]

On 1/5/15, 3:14 PM, Tom Lane wrote:

Jim Nasby  writes:

Related... I'd like to see a way to inline a function that does something like:



CREATE FUNCTION foo(text) RETURNS int LANGUAGE sql AS $$
SELECT a FROM b WHERE lower(b.c) = lower($1)
$$


The reason that's not inlined ATM is that the semantics wouldn't be the
same (ie, what happens if the SELECT returns more than one row).  It's
possible they would be the same if we attached a LIMIT 1 to the function's
query, but I'm not 100% sure about that offhand.  I'm also not real sure
that you'd still get good performance if there were an inserted LIMIT;
that would disable at least some optimizations.


In this case there's actually a unique index on lower(b.c). I don't know if the 
planner is smart enough to recognize that today though.

Perhaps a good interim solution would be a flag/option you could set on SQL 
functions to force (or disallow) inlining? That means if the option is set it's 
on the callers head if it doesn't do what's desired. We should throw an error 
if there's something about the function that would prevent inlining though.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Jim Nasby]

On 1/6/15, 10:32 AM, Alvaro Herrera wrote:

Tom Lane wrote:


What would make sense to me is to teach the planner about inlining
SQL functions that include ORDER BY clauses, so that the performance
issue of a double sort could be avoided entirely transparently to
the user.


Wouldn't this be applicable to functions in other languages too, not
only SQL?


Dumb question... we can inline functions from other languages? What chunk of 
code handles that?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] Add min and max execute statement time in pg_stat_statement

[Andrew Dunstan]

On 12/21/2014 02:50 PM, Andrew Dunstan wrote:


On 12/21/2014 02:12 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 12/21/2014 01:23 PM, Alvaro Herrera wrote:
The point, I think, is that without atomic instructions you have to 
hold

a lock while incrementing the counters.

Hmm, do we do that now?

We already have a spinlock mutex around the counter adjustment code, so
I'm not sure why this discussion is being held.


Because Peter suggested we might be able to use atomics. I'm a bit 
dubious that we can for min and max anyway.





I would like someone more versed in numerical analysis than me to
tell me how safe using sum of squares actually is in our case.

That, on the other hand, might be a real issue.  I'm afraid that
accumulating across a very long series of statements could lead
to severe roundoff error in the reported values, unless we use
a method chosen for numerical stability.





Right.

The next question along those lines is whether we need to keep a 
running mean or whether that can safely be calculated on the fly. The 
code at  does keep 
a running mean, and maybe that's required to prevent ill conditioned 
results, although I'm quite sure I see how it would. But this isn't my 
area of expertise.





I played it safe and kept a running mean, and since it's there and 
useful in itself I exposed it via the function, so there are four new 
columns, min_time, max_time, mean_time and stddev_time.


I'll add this to the upcoming commitfest.

cheers

andrew
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 2709909..975a637 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,8 +4,9 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.2.sql pg_stat_statements--1.1--1.2.sql \
-	pg_stat_statements--1.0--1.1.sql pg_stat_statements--unpackaged--1.0.sql
+DATA = pg_stat_statements--1.3.sql pg_stat_statements--1.2--1.3.sql \
+	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
+	pg_stat_statements--unpackaged--1.0.sql
 PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
 
 ifdef USE_PGXS
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.2-1.3.sql b/contrib/pg_stat_statements/pg_stat_statements--1.2-1.3.sql
new file mode 100644
index 000..506959d
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.2-1.3.sql
@@ -0,0 +1,47 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.2--1.3.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.3'" to load this file. \quit
+
+/* First we have to remove them from the extension */
+ALTER EXTENSION pg_stat_statements DROP VIEW pg_stat_statements;
+ALTER EXTENSION pg_stat_statements DROP FUNCTION pg_stat_statements();
+
+/* Then we can drop them */
+DROP VIEW pg_stat_statements;
+DROP FUNCTION pg_stat_statements();
+
+/* Now redefine */
+CREATE FUNCTION pg_stat_statements(IN showtext boolean,
+OUT userid oid,
+OUT dbid oid,
+OUT queryid bigint,
+OUT query text,
+OUT calls int8,
+OUT total_time float8,
+OUT min_time float8,
+OUT max_time float8,
+OUT mean_time float8,
+OUT stddev_time float8,
+OUT rows int8,
+OUT shared_blks_hit int8,
+OUT shared_blks_read int8,
+OUT shared_blks_dirtied int8,
+OUT shared_blks_written int8,
+OUT local_blks_hit int8,
+OUT local_blks_read int8,
+OUT local_blks_dirtied int8,
+OUT local_blks_written int8,
+OUT temp_blks_read int8,
+OUT temp_blks_written int8,
+OUT blk_read_time float8,
+OUT blk_write_time float8
+)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'pg_stat_statements_1_3'
+LANGUAGE C STRICT VOLATILE;
+
+CREATE VIEW pg_stat_statements AS
+  SELECT * FROM pg_stat_statements(true);
+
+GRANT SELECT ON pg_stat_statements TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.2.sql b/contrib/pg_stat_statements/pg_stat_statements--1.2.sql
deleted file mode 100644
index 5bfa9a5..000
--- a/contrib/pg_stat_statements/pg_stat_statements--1.2.sql
+++ /dev/null
@@ -1,44 +0,0 @@
-/* contrib/pg_stat_statements/pg_stat_statements--1.2.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION pg_stat_statements" to load this file. \quit
-
--- Register functions.
-CREATE FUNCTION pg_stat_statements_reset()
-RETURNS void
-AS 'MODULE_PATHNAME'
-LANGUAGE C;
-
-CREATE FUNCTION pg_stat_statements(IN showtext boolean,
-OUT userid oid,
-OUT dbid oid,
-OUT queryid bigint,
-OUT query text,
-OUT calls int8,
-OUT total_time float8,
-OUT rows int8,
-OUT shared_blks_hit int8,
-OUT shared_blks_read int8,
-OUT shared_blks_dirtied int8,
-OUT shared_blks_written int8,

Re: [HACKERS] Possible typo in create_policy.sgml

[Peter Geoghegan]

I also don't see this behavior documented (this is from process_policies()):

/*
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
if (with_check_quals == NIL)
with_check_quals = copyObject(quals);

Now, I do see a reference to it under "Per-Command policies - ALL". It says:

"If an INSERT or UPDATE command attempts to add rows to the table
which do not pass the ALL WITH CHECK (or USING, if no WITH CHECK
expression is defined) expression, the command will error."

But is that really the right place for it? Does it not equally well
apply to FOR UPDATE policies, that can on their own have both barriers
quals and WITH CHECK options? Basically, that seems to me like a
*generic* property of policies (it's a generic thing that the WITH
CHECK options/expressions "shadow" the USING security barrier quals as
check options), and so should be documented as such.

-- 
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: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Petr Jelinek]

On 07/01/15 00:05, Jim Nasby wrote:

On 1/6/15, 8:17 AM, Kouhei Kaigai wrote:

The attached patch is newer revision of custom-/foreign-join
interface.


Shouldn't instances of

scan_relid > 0

be

scan_relid != InvalidOid



Ideally, they should be OidIsValid(scan_relid)


--
 Petr Jelinek  http://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] Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Jim Nasby]

On 1/6/15, 1:00 AM, Ashutosh Bapat wrote:


Even checking whether the output of the function is in the right order or not, 
has its cost. I am suggesting that we can eliminate this cost as well. For 
example, PostgreSQL does not check whether a function is really immutable or 
not.


Actually, it does:

select test();
ERROR:  UPDATE is not allowed in a non-volatile function
CONTEXT:  SQL statement "UPDATE i SET i=i+1"
PL/pgSQL function test() line 3 at SQL statement
STATEMENT:  select test();
ERROR:  UPDATE is not allowed in a non-volatile function
CONTEXT:  SQL statement "UPDATE i SET i=i+1"
PL/pgSQL function test() line 3 at SQL statement

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


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

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Kouhei Kaigai]

> > scan_relid != InvalidOid
> >
> 
> Ideally, they should be OidIsValid(scan_relid)
>
Scan.scanrelid is an index of range-tables list, not an object-id.
So, InvalidOid or OidIsValid() are not a good choice.

The bare relation oid has to be saved on relid of RangeTblEntry
which can be pulled using rt_fetch(scanrelid, range_tables).

I could found an assertion below at ExecScanFetch().
  Assert(scanrelid > 0);
Probably, it is a usual manner for this.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei 


> -Original Message-
> From: Petr Jelinek [mailto:p...@2ndquadrant.com]
> Sent: Wednesday, January 07, 2015 8:24 AM
> To: Jim Nasby; Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada
> Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan
> API)
> 
> On 07/01/15 00:05, Jim Nasby wrote:
> > On 1/6/15, 8:17 AM, Kouhei Kaigai wrote:
> >> The attached patch is newer revision of custom-/foreign-join
> >> interface.
> >
> > Shouldn't instances of
> >
> > scan_relid > 0
> >
> > be
> >
> > scan_relid != InvalidOid
> >
> 
> Ideally, they should be OidIsValid(scan_relid)
> 
> 
> --
>   Petr Jelinek  http://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] pg_rewind in contrib

[Michael Paquier]

On Wed, Jan 7, 2015 at 5:39 AM, Peter Eisentraut  wrote:
> Also, since you have been maintaining this tool for a while, what is the
> effort for maintaining it from version to version?
>From my own experience, the main source of maintenance across major
versions is the modification of the WAL record format to be able to
track the blocks that need to be copied from the newly promoted master
to the node rewound. That has been an ongoing effort on REL9_4_STABLE
and REL9_3_STABLE since this tool has been created when both versions
were in development to keep the tool compatible all the time. This
problem does not exist anymore on master thanks to the new WAL format
able to track easily the blocks modified, limiting the maintenance
necessary to actual bugs.
-- 
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] Updating copyright notices to 2015 for PGDG

[Jim Nasby]

On 1/6/15, 3:30 PM, Stefan Kaltenbrunner wrote:

I dont know why it is really needed but maybe for the files that have
identical copyrights one could simple reference to the COPYRIGHT file we
already have in the tree?


+1

Also, now that we're on git it wouldn't be that hard to add commit hooks that 
prevent (or maybe even fix) trailing LF. If this is somethin pg_indent or other 
tools would do anyway ISTM it'd be nice to use a hook that fixes it because it 
would cut down on the size of pg_indent diffs.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] pg_rewind in contrib

[Andrew Dunstan]

On 01/06/2015 03:39 PM, Peter Eisentraut wrote:

I applaud the ingenuity on all levels in this patch.  But it seems to me
that there is way too much backend knowledge encoded and/or duplicated
in a front-end program.

If this ends up shipping, it's going to be a massively popular tool.  I
see it as a companion to pg_basebackup.  So it should sort of work the
same way.  One problem is that it doesn't use the replication protocol,
so the setup is going to be inconsistent with pg_basebackup.  Maybe the
replication protocol could be extended to provide the required data.
Maybe something as simple as "give me this file" would work.

That might lose the local copy mode, but how important is that?
pg_basebackup doesn't have that mode.  In any case, the documentation
doesn't explain this distinction.  The option documentation is a bit
short in any case, but it's not clear that you can choose between local
and remote mode.

The test suite should probably be reimplemented in Perl.  (I might be
able to help.)  Again, ingenious, but it's very hard to follow the
sequence of what is being tested.  And some Windows person is going to
complain. ;-)

Also, since you have been maintaining this tool for a while, what is the
effort for maintaining it from version to version?





I also think it's a great idea. But I think we should consider the name 
carefully. pg_resync might be a better name. Strictly, you might not be 
quite rewinding, AIUI.


cheers

andrew



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

Re: [HACKERS] Updating copyright notices to 2015 for PGDG

[Michael Paquier]

On Wed, Jan 7, 2015 at 8:48 AM, Jim Nasby  wrote:
> On 1/6/15, 3:30 PM, Stefan Kaltenbrunner wrote:
>>
>> I dont know why it is really needed but maybe for the files that have
>> identical copyrights one could simple reference to the COPYRIGHT file we
>> already have in the tree?
>
>
> +1
>
> Also, now that we're on git it wouldn't be that hard to add commit hooks
> that prevent (or maybe even fix) trailing LF. If this is somethin pg_indent
> or other tools would do anyway ISTM it'd be nice to use a hook that fixes it
> because it would cut down on the size of pg_indent diffs.
... And increase the size of vanilla commits if you do it in the same
commit, or make the git history less readable if you fix them as a
separate commit. It is better IMO to keep the cleanup work in a single
huge commit.
-- 
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] XLOG_PARAMETER_CHANGE handling of wal_log_hints

[Michael Paquier]

On Wed, Jan 7, 2015 at 4:24 AM, Petr Jelinek  wrote:
> Hi,
>
> when I was fixing how commit_ts handles the XLOG_PARAMETER_CHANGE I noticed
> that for wal_log_hints we assign the value in ControFile to current value
> instead of value that comes from WAL.
>
> ISTM it has just information value so it should not have any practical
> impact, but it looks like a bug anyway so here is simple patch to change
> that.
Right. That's something that should get fixed in 9.4 as well..

 ControlFile->track_commit_timestamp = track_commit_timestamp;
The new value of track_commit_timestamp should be taken as well from
xlrec on master btw.
-- 
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] Updating copyright notices to 2015 for PGDG

[Tom Lane]

Jim Nasby  writes:
> On 1/6/15, 3:30 PM, Stefan Kaltenbrunner wrote:
>> I dont know why it is really needed but maybe for the files that have
>> identical copyrights one could simple reference to the COPYRIGHT file we
>> already have in the tree?

> +1

Unless either of you is a copyright lawyer, your opinion on whether that's
sufficient is of zero relevance.

Personally I think it's just fine if we have some mechanism that forces
text files to have trailing newlines ;-)

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] pg_rewind in contrib

[Andres Freund]

On 2015-01-06 15:39:29 -0500, Peter Eisentraut wrote:
> I applaud the ingenuity on all levels in this patch.  But it seems to me
> that there is way too much backend knowledge encoded and/or duplicated
> in a front-end program.

Hm. There's really not that much in the current version anymore? Sure,
there's some xlog record specific knowledge, some about the whole data
directory layout and a bunch of timeline specific stuff. But that's not
that much.

Don't get me wrong - I personally think this shouldn't be in contrib but
in bin.  The amount of prerequisite work to allow this to be
maintainable (2c03216d, 2076db2, ...) is a hint of how closely this is
linked and how much effort core community people have put into this.  I
don't think contrib/ is the right place for that. Even though we haven't
found something we can agree on wrt moving other stuff (apprently at
least?) from contrib, we can still place new stuff in src/bin instead of
contrib.


It wouldn't hurt if we could share SimpleXLogPageRead() between
pg_xlogdump and pg_rewind as the differences are more or less
superficial, but that seems simple enough to achieve by putting a
frontend variant in xlogreader.c/h.

> If this ends up shipping, it's going to be a massively popular tool.  I
> see it as a companion to pg_basebackup.  So it should sort of work the
> same way.  One problem is that it doesn't use the replication protocol,
> so the setup is going to be inconsistent with pg_basebackup.  Maybe the
> replication protocol could be extended to provide the required data.

I'm not particularly bothered by the requirement of also requiring a
normal, not replication, connection. In most cases that'll already be
allowed.

> Maybe something as simple as "give me this file" would work.

Well, looking at libpq_fetch.c it seems there'd be a bit more required.

Not having to create a temporary table on the other side would be nice -
afaics there's otherwise not much stopping this from working against a
standby?

> That might lose the local copy mode, but how important is that?
> pg_basebackup doesn't have that mode.

But we have plain pg_start/stop_backup for that case. That alternative
doesn't really exist here.

Greetings,

Andres Freund

-- 
 Andres Freund http://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] Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[David Johnston]

On Tue, Jan 6, 2015 at 4:15 PM, Jim Nasby  wrote:

> On 1/6/15, 10:32 AM, Alvaro Herrera wrote:
>
>> Tom Lane wrote:
>>
>>  What would make sense to me is to teach the planner about inlining
>>> SQL functions that include ORDER BY clauses, so that the performance
>>> issue of a double sort could be avoided entirely transparently to
>>> the user.
>>>
>>
>> Wouldn't this be applicable to functions in other languages too, not
>> only SQL?
>>
>
> Dumb question... we can inline functions from other languages? What chunk
> of code handles that?


​We cannot that I know of.  The point being made here is that suggesting an
alternative that requires inlining ​doesn't cover the entire purpose of
this feature since the feature can be applied to functions that cannot be
inlined.

David J.

Re: [HACKERS] Turning recovery.conf into GUCs

[Josh Berkus]

Peter, all:

Let me go over the issues I find with recovery.conf, based on 3 aspects
of my experience (1) doing DBA support (2) as a tool author (HandyRep)
and (3) as a trainer, teaching new users about it.  If we agree on a
list of problems with the current setup (as well as a list of benefits)
that's a good litmus test on whether any new version is worth adopting.
 Basically, new ways of doing this should remove some of the issues
while not jettisoning the benefits as much as possible.

Issues:

A. different formatting compared with PostgreSQL.conf, particularly
quoting, and lack of support for include files.

B. Inability to find out recovery.conf variables over port 5432.

C. Difficulty of management due to being both a trigger file and a
configuration file.

D. Wierd name: for those doing only replication, not PITR,
"recovery.conf" is completely baffling.

E. Replication/PITR confusion: some configuration items (particularly
recovery_target_*) are contradictory.

F. Inability to remaster without restarting the replica.

G. Inability to change parameters using ALTER SYSTEM SET.

H. Requirement of being in the data directory instead of the
configuration directory.

I. Fairly duplicative options between pg.conf and recovery.conf (i.e.
"standby_mode" vs. "hot_standby")

Benefits:

1. Ability to share the exact same postgresql.conf for replica and master.

2. Easy pg_basebackup because you can exclude/generate a recovery.conf
automatically.

3. Battle-tested way to make sure that replication/recovery state
persists across unexpected restarts, and simple promotion workflow.


-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Turning recovery.conf into GUCs

[Andres Freund]

On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
> F. Inability to remaster without restarting the replica.

That has pretty much nothing to do with the topic at hand.

> I. Fairly duplicative options between pg.conf and recovery.conf (i.e.
> "standby_mode" vs. "hot_standby")

Those aren't the same.

Greetings,

Andres Freund

-- 
 Andres Freund http://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] Turning recovery.conf into GUCs

[Josh Berkus]

On 01/06/2015 04:42 PM, Andres Freund wrote:
> On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
>> F. Inability to remaster without restarting the replica.
> 
> That has pretty much nothing to do with the topic at hand.

It has *everything* to do with the topic at hand.  The *only* reason we
can't remaster without restarting is that recovery.conf is only read at
startup time, and can't be reloaded.

http://www.databasesoup.com/2014/05/remastering-without-restarting.html

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Turning recovery.conf into GUCs

[Andres Freund]

On 2015-01-05 20:43:12 -0500, Peter Eisentraut wrote:
> For example, putting recovery target parameters into postgresql.conf
> might not make sense to some people.  Once you have reached the recovery
> end point, the information is obsolete.  Keeping it set could be
> considered confusing.

I don't know, but I think that ship has sailed. hot_standby,
archive_command, archive_mode, hot_standby_feedback are all essentially
treated differently between primary and standby.

> Moreover, when I'm actually doing point-in-time-recovery attempts, I
> don't think I want to be messing with my pristine postgresql.conf file.
>  Having those settings separate isn't a bad idea in that case.

Well, nothing stops you from having a include file or something similar.

I think we should just make recovery.conf behave exactly the way it does
right now, except parse it according to guc rules. That way the changes
when migrating are minimal and we don't desupport any
usecases. Disallowing that way of operating just seems like
intentionally throwing rocks in the way of getting this done.

> In the past, when some people have complained that recovery.conf cannot
> be moved to a configuration directory, I (and others?) have argued that
> recovery.conf is really more of a command script file than a
> configuration file (that was before replication was commonplace).  The
> premise of this patch is that some options really are more configuration
> than command most of the time, but that doesn't mean the old view was
> invalid.

Again, I think this ship has long since sailed.

> The current system makes it easy to share postgresql.conf between
> primary and standby and just maintain the information related to the
> standby locally in recovery.conf.  pg_basebackup -R makes that even
> easier.  It's still possible to do this in the new system, but it's
> decidedly more work.

Really? Howso?

> The wins on the other hand are obscure: You can now use SHOW to inspect
> recovery settings.  You can design your own configuration file include
> structures to set them.  These are not bad, but is that all?

It's much more:
a) One configuration format instead of two somewhat, but not really,
   similar ones.
b) Proper infrastructure to deal with configuration variable boundaries
   and such. Just a few days ago there was e7c11887 et al.
c) Infrastructure for changing settings effective during recovery. Right
   now we'd have to rebuild a lot of guc infrasturcture to allow
   that. It'd not be that hard to allow changing parameters like
   restore_command, primary_conninfo, recovery_target_* et al. That's
   for sure not the same commit, but once the infrastructure is in those
   won't be too hard.

> I note that all the new settings are PGC_POSTMASTER, so you can't set
> them on the fly.  Changing primary_conninfo without a restart would be a
> big win, for example.  Is that planned?

I think it's not too hard to do - but I'll fight hard to do that
separately. Once we've the infrastructure I'd be surprised if took too
long to change some of them to PGC_SIGHUP.

> I have previously argued for a different approach: Ready recovery.conf
> as a configuration file after postgresql.conf, but keep it as a file to
> start recovery.  That doesn't break any old workflows, it gets you all
> the advantages of having recovery parameters in the GUC system, and it
> keeps all the options open for improving things further in the future.

Well, it breaks because quoting and such is different. Otherwise I think
I agree. It allows you to keep parameters in recovery.conf if you want,
if not not.

If we add a recovery_file guc that defaults to '$PGDATA/recovery.conf'
we'd have a rather easy way to move forward imo. We can even allow two
filenames so we could default to something like
recovery_file = 'PGDATA/recovery.conf; PGDATA/recovery.trigger'

Greetings,

Andres Freund

-- 
 Andres Freund http://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] Turning recovery.conf into GUCs

[Andres Freund]

On 2015-01-06 17:08:20 -0800, Josh Berkus wrote:
> On 01/06/2015 04:42 PM, Andres Freund wrote:
> > On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
> >> F. Inability to remaster without restarting the replica.
> > 
> > That has pretty much nothing to do with the topic at hand.
> 
> It has *everything* to do with the topic at hand.  The *only* reason we
> can't remaster without restarting is that recovery.conf is only read at
> startup time, and can't be reloaded.

> http://www.databasesoup.com/2014/05/remastering-without-restarting.html

Not really. It's a good way to introduce suble and hard to understand
corruption and other strange corner cases. Your replication connection
was lost/reconnected in the wrong moment? Oops, received/replayed too
much.

To achieve what you describe there, you don't even need a proxy, simple
dns based failover suffices.

A real solution to this requires more. You need to 1) disable writing
any wal 2) force catchup of all possible standbys, including apply. Most
importantly of the new master. This requires knowing which standbys
exist. 3) promote new master. 4) only now allow reconnects.

Greetings,

Andres Freund

-- 
 Andres Freund http://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] Possible typo in create_policy.sgml

[Amit Langote]

On 07-01-2015 AM 04:25, Stephen Frost wrote:
> Robert, Amit,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
>> I don't think that's a typo, although it's not particularly
>> well-worded IMHO.  I might rewrite the whole paragraph like this:
>>
>> A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
>> in a table to those rows which match the relevant policy expression.
>> Existing table rows are checked against the expression specified via
>> USING, while new rows that would be created via INSERT or UPDATE are
>> checked against the expression specified via WITH CHECK.  Generally,
>> the system will enforce filter conditions imposed using security
>> policies prior to qualifications that appear in the query itself, in
>> order to the prevent the inadvertent exposure of the protected data to
>> user-defined functions which might not be trustworthy.  However,
>> functions and operators marked by the system (or the system
>> administrator) as LEAKPROOF may be evaluated before policy
>> expressions, as they are assumed to be trustworthy.
> 
> Looks reasonable to me.  Amit, does this read better for you?  If so, I
> can handle making the change to the docs.
> 

Yes, it looks reasonable to me to.

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] Turning recovery.conf into GUCs

[Josh Berkus]

On 01/06/2015 05:17 PM, Andres Freund wrote:
> A real solution to this requires more. You need to 1) disable writing
> any wal 2) force catchup of all possible standbys, including apply. Most
> importantly of the new master. This requires knowing which standbys
> exist. 3) promote new master. 4) only now allow reconnect

That can all be handled externally to PostgreSQL.  However, as long as
we have a recovery.conf file which only gets read at server start, and
at no other time, no external solution is possible.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Updating copyright notices to 2015 for PGDG

[Alvaro Herrera]

Stefan Kaltenbrunner wrote:
> On 01/06/2015 10:12 PM, Alvaro Herrera wrote:

> > This makes me wonder however how wise it is to update the copyright
> > notices in every single file in the repo.  Why do we need this?  Why not
> > abolish the practice and live forever with most files having copyright
> > 2015?  (Only new files would have newer years in their copyright
> > notices, I guess.)  Does this provide us with any kind of protection,
> > and if so against what, and how does it protect us?  Since we have a
> > very clean git history which shows us the exact provenance of every
> > single line of source code, and we have excellent mail archives that
> > show where each line came from for all development in the last decade,
> > this single line of (C) boilerplate in each file seems completely
> > pointless.
> 
> I dont know why it is really needed but maybe for the files that have
> identical copyrights one could simple reference to the COPYRIGHT file we
> already have in the tree?

+1 to that, but I would +2 a script that just did
g/Copyright Regents of Fooniversity/d
etc.

Abhijit et al will probably hate me for referencing this, but here it
goes anyway:
http://toroid.org/ams/etc/updating-copyright-notices

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


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

Re: [HACKERS] parallel mode and parallel contexts

[Jim Nasby]

On 1/6/15, 10:33 AM, Robert Haas wrote:

>Entrypints?

Already noted by Andres; fixed in the attached version.


Perhaps we only parallelize while drinking beer... ;)

CreateParallelContext(): Does it actually make sense to have nworkers=0? ISTM 
that's a bogus case. Also, since the number of workers will normally be 
determined dynamically by the planner, should that check be a regular 
conditional instead of just an Assert?

In LaunchParallelWorkers() the "Start Workers" comment states that we give up 
registering more workers if one fails to register, but there's nothing in the if 
condition to do that, and I don't see RegisterDynamicBackgroundWorker() doing it either. 
Is the comment just incorrect?

SerializeTransactionState(): instead of looping through the transaction stack 
to calculate nxids, couldn't we just set it to maxsize - sizeof(TransactionId) 
* 3? If we're looping a second time for safety a comment explaining that would 
be useful...

sequence.c: Is it safe to read a sequence value in a parallel worker if the 
cache_value is > 1?

This may be a dumb question, but for functions do we know that all pl's besides 
C and SQL use SPI? If not I think they could end up writing in a worker.

@@ -2968,7 +2969,8 @@ ProcessInterrupts(void)
 errmsg("canceling statement due to user 
request")));
}
}
-   /* If we get here, do nothing (probably, QueryCancelPending was reset) 
*/
+   if (ParallelMessagePending)
+   HandleParallelMessageInterrupt(false);
ISTM it'd be good to leave that comment in place (after the if).

RestoreComboCIDState(): Assert(!comboCids || !comboHash): Shouldn't that be &&? 
AIUI both should always be either set or 0.

Typo: + elog(ERROR, "cannot update SecondarySnapshpt during a parallel 
operation");

The comment for RestoreSnapshot refers to set_transaction_snapshot, but that 
doesn't actually exist (or isn't referenced).
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Amit Langote]

On 07-01-2015 AM 08:33, Jim Nasby wrote:
> On 1/6/15, 1:00 AM, Ashutosh Bapat wrote:
>>
>> Even checking whether the output of the function is in the right order
>> or not, has its cost. I am suggesting that we can eliminate this cost
>> as well. For example, PostgreSQL does not check whether a function is
>> really immutable or not.
> 
> Actually, it does:
> 
> select test();
> ERROR:  UPDATE is not allowed in a non-volatile function
> CONTEXT:  SQL statement "UPDATE i SET i=i+1"
> PL/pgSQL function test() line 3 at SQL statement
> STATEMENT:  select test();
> ERROR:  UPDATE is not allowed in a non-volatile function
> CONTEXT:  SQL statement "UPDATE i SET i=i+1"
> PL/pgSQL function test() line 3 at SQL statement
> 

I think Ashutosh's point is that there is no dedicated executor node to
perform this check. ISTM, the above error is raised during planning
itself as part of the initialization of state for a function.

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] pg_basebackup fails with long tablespace paths

[Amit Kapila]

On Wed, Jan 7, 2015 at 3:03 AM, Peter Eisentraut  wrote:
>
> On 12/27/14 8:02 PM, Robert Haas wrote:
> > On Wed, Dec 24, 2014 at 8:12 AM, Peter Eisentraut 
wrote:
> >> On 12/22/14 10:00 PM, Amit Kapila wrote:
> >>> There is already a patch [1] in this CF which will handle both cases,
so
> >>> I am
> >>> not sure if it is very good idea to go with a new tar format to
handle this
> >>> issue.
> >>
> >> I think it would still make sense to have proper symlinks in the
> >> basebackup if possible, for clarity.
> >
> > I guess I would have assumed it would be more clear to omit the
> > symlinks if we're expecting the server to put them in.  Otherwise, the
> > server has to remove the existing symlinks and create new ones, which
> > introduces various possibilities for failure and confusion.
>
> Currently, when you unpack a tarred basebackup with tablespaces, the
> symlinks will tell you whether you have unpacked the tablespace tars at
> the right place.  Otherwise, how do you know?

via some kind of tablespace map file which will tell us the exact
location where symlink need to be pointed and the same will be used
to create a symlink.  So after you unpack a tarred basebackup with
tablespaces, there will be no symlinks; when you start the server
(archive recovery) using base backup, it will create the appropriate
symlinks.

> Secondly, you also have
> the option of putting the tablespaces somewhere else by changing the
> symlinks.  Under the new scheme, the existing symlinks would be
> overwritten (or not?).  If that is actually correct, then the proposed
> fix doesn't really replicate the required functionality on Windows.
>
> One way to address this would be to do away with the symlinks altogether
> and have pg_tblspc/12345 be a text file that contains the tablespace
> location.  Kind of symlinks implemented in user space.
>

I think this is somewhat similar to what existing patch [1] does with
the different that there is just one file for all the tablespace locations
rather than individual file in each tablespace directory.


[1] : https://commitfest.postgresql.org/action/patch_view?id=1512
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

[Michael Paquier]

On Wed, Jan 7, 2015 at 12:51 AM, Rahila Syed  wrote:
> Following are some comments,
Thanks for the feedback.

>>uint16  hole_offset:15, /* number of bytes in "hole" */
> Typo in description of hole_offset
Fixed. That's "before hole".

>>for (block_id = 0; block_id <= record->max_block_id; block_id++)
>>-   {
>>-   if (XLogRecHasBlockImage(record, block_id))
>>-   fpi_len += BLCKSZ -
> record->blocks[block_id].hole_length;
>>-   }
>>+   fpi_len += record->blocks[block_id].bkp_len;
>
> IIUC, if condition, /if(XLogRecHasBlockImage(record, block_id))/ is
> incorrectly removed from the above for loop.
Fixed.

>>typedef struct XLogRecordCompressedBlockImageHeader
> I am trying to understand the purpose behind declaration of the above
> struct. IIUC, it is defined in order to introduce new field uint16
> raw_length and it has been declared as a separate struct from
> XLogRecordBlockImageHeader to not affect the size of WAL record when
> compression is off.
> I wonder if it is ok to simply memcpy the uint16 raw_length in the
> hdr_scratch when compression is on
> and not have a separate header struct for it neither declare it in existing
> header. raw_length can be a locally defined variable is XLogRecordAssemble
> or it can be a field in registered_buffer struct like compressed_page.
> I think this can simplify the code.
> Am I missing something obvious?
You are missing nothing. I just introduced this structure for a matter
of readability to show the two-byte difference between non-compressed
and compressed header information. It is true that doing it my way
makes the structures duplicated, so let's simply add the
compression-related information as an extra structure added after
XLogRecordBlockImageHeader if the block is compressed. I hope this
addresses your concerns.

>> /*
>>  * Fill in the remaining fields in the XLogRecordBlockImageHeader
>>  * struct and add new entries in the record chain.
>> */
>
>>   bkpb.fork_flags |= BKPBLOCK_HAS_IMAGE;
>
> This code line seems to be misplaced with respect to the above comment.
> Comment indicates filling of XLogRecordBlockImageHeader fields while
> fork_flags is a field of XLogRecordBlockHeader.
> Is it better to place the code close to following condition?
>   if (needs_backup)
>   {
Yes, this comment should not be here. I replaced it with the comment in HEAD.


>>+  *the original length of the
>>+ * block without its page hole being deducible from the compressed data
>>+ * itself.
> IIUC, this comment before XLogRecordBlockImageHeader seems to be no longer
> valid as original length is not deducible from compressed data and rather
> stored in header.
Aah, true. This was originally present in the header of PGLZ that has
been removed to make it available for frontends.

Updated patches are attached.
Regards,
-- 
Michael


20150107_fpw_compression_v14.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Ashutosh Bapat]

Not in all cases

postgres=# create function non_im_immutable_function() returns float as $$
begin
return *random()*;
end;
$$ language plpgsql *immutable*;
CREATE FUNCTION

postgres=# select proname, provolatile from pg_proc where proname =
'random' or proname = 'non_im_immutable_function';
  proname  | provolatile
---+-
 random| v
 non_im_immutable_function | i

postgres=# select non_im_immutable_function();
 non_im_immutable_function
---
 0.963812265079468
(1 row)
postgres=# select non_im_immutable_function();
 non_im_immutable_function
---
 0.362834882922471
(1 row)

Per definition of immutable functions, the function's output shouldn't
depend upon a volatile function e.g. random().

On Wed, Jan 7, 2015 at 5:03 AM, Jim Nasby  wrote:

> On 1/6/15, 1:00 AM, Ashutosh Bapat wrote:
>
>>
>> Even checking whether the output of the function is in the right order or
>> not, has its cost. I am suggesting that we can eliminate this cost as well.
>> For example, PostgreSQL does not check whether a function is really
>> immutable or not.
>>
>
> Actually, it does:
>
> select test();
> ERROR:  UPDATE is not allowed in a non-volatile function
> CONTEXT:  SQL statement "UPDATE i SET i=i+1"
> PL/pgSQL function test() line 3 at SQL statement
> STATEMENT:  select test();
> ERROR:  UPDATE is not allowed in a non-volatile function
> CONTEXT:  SQL statement "UPDATE i SET i=i+1"
> PL/pgSQL function test() line 3 at SQL statement
>
>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] [REVIEW] Re: Fix xpath() to return namespace definitions

[Peter Eisentraut]

committed version 7


-- 
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 to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

[Michael Paquier]

On Tue, Jan 6, 2015 at 12:12 AM, Atri Sharma  wrote:
> I will add the patch to current commitfest.
It has been indeed added to the commit fest 2014-12. That's a bit
late, moving it to upcoming one 2015-02.
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] pg_basebackup vs. Windows and tablespaces

[Dilip kumar]

On 20 December 2014 16:30, Amit Kapila Wrote,


>Summarization of latest changes:
>1. Change file name from symlink_label to tablespace_map and changed
>the same every where in comments and variable names.
>2. This feature will be supportted for both windows and linux; tablespace_map
>file will be generated on both windows and linux to restore tablespace links
>during archive recovery.
>3.  Handling for special characters in tablesapce path name.
>4. Updation of docs.

I did not followed this patch for quite some time, I have seen all the threads 
regarding this patch and reviewed from those perspective.


1. I have done the testing and behavior is fine

2. For handling special character like new line character, I saw discussion 
mostly for two option,

a. Don’t support such table space name in tablespace map file and skip 
those tablespace.

b. Add ‘\’ character when there is new line in the tablespace name.



And you have selected option 2, I don’t see any problem in this because it is 
maintaining human readability, I just want ask is this as per the consensus ?


Other than that patch seems fine to me..

Regards,
Dilip

Re: [HACKERS] pg_basebackup vs. Windows and tablespaces

[Amit Kapila]

On Wed, Jan 7, 2015 at 10:45 AM, Dilip kumar  wrote:
>
> On 20 December 2014 16:30, Amit Kapila Wrote,
> >Summarization of latest changes:
>
> >1. Change file name from symlink_label to tablespace_map and changed
>
> >the same every where in comments and variable names.
>
> >2. This feature will be supportted for both windows and linux;
tablespace_map
>
> >file will be generated on both windows and linux to restore tablespace
links
>
> >during archive recovery.
>
> >3.  Handling for special characters in tablesapce path name.
> >4. Updation of docs.
>
> I did not followed this patch for quite some time, I have seen all the
threads regarding this patch and reviewed from those perspective.
>
> 1. I have done the testing and behavior is fine
> 2. For handling special character like new line character, I saw
discussion mostly for two option,
> a. Don’t support such table space name in tablespace map file and
skip those tablespace.
>
> b. Add ‘\’ character when there is new line in the tablespace name.
>
> And you have selected option 2, I don’t see any problem in this because
it is maintaining human readability, I just want ask is this as per the
consensus ?
>

Tom has spotted this problem and suggested 3 different options
to handle this issue, apart from above 2, third one is "Go over to
a byte-count-then-value format".  Then Andrew and Heikki
supported/asked to follow option 2 (as is followed in patch) and no
one objected, so I used the same to fix the issue.

Based on above, I would say we have a consensus to follow this
approach.

>
> Other than that patch seems fine to me..
>

Thanks for reviewing it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] pg_basebackup vs. Windows and tablespaces

[Dilip kumar]

On 07 January 2015 11:21, Amit Kapila Wrote,

>Tom has spotted this problem and suggested 3 different options
>to handle this issue, apart from above 2, third one is "Go over to
>a byte-count-then-value format".  Then Andrew and Heikki
>supported/asked to follow option 2 (as is followed in patch) and no
>one objected, so I used the same to fix the issue.

>Based on above, I would say we have a consensus to follow this
>approach.

Moved to Ready For Committer

Regards,
Dilip

Re: [HACKERS] [REVIEW] Re: Fix xpath() to return namespace definitions

[Tom Lane]

Peter Eisentraut  writes:
> committed version 7

Isn't that a back-patchable bug fix?

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] Transactions involving multiple postgres foreign servers

[Ashutosh Bapat]

On Mon, Jan 5, 2015 at 11:55 PM, Robert Haas  wrote:

> On Fri, Jan 2, 2015 at 3:45 PM, Tom Lane  wrote:
> > In short, you can't force 2PC technology on people who aren't using it
> > already; while for those who are using it already, this isn't nearly
> > good enough as-is.
>
> I was involved in some internal discussions related to this patch, so
> I have some opinions on it.  The long-term, high-level goal here is to
> facilitate sharding.  If we've got a bunch of PostgreSQL servers
> interlinked via postgres_fdw, it should be possible to perform
> transactions on the cluster in such a way that transactions are just
> as atomic, consistent, isolated, and durable as they would be with
> just one server.  As far as I know, there is no way to achieve this
> goal through the use of an external transaction manager, because even
> if that external transaction manager guarantees, for every
> transaction, that the transaction either commits on all nodes or rolls
> back on all nodes, there's no way for it to guarantee that other
> transactions won't see some intermediate state where the commit has
> been completed on some nodes but not others.  To get that, you need
> some of integration that reaches down to the way snapshots are taken.
>
> I think, though, that it might be worthwhile to first solve the
> simpler problem of figuring out how to ensure that a transaction
> commits everywhere or rolls back everywhere, even if intermediate
> states might still be transiently visible.


Agreed.


> I don't think this patch,
> as currently designed, is equal to that challenge, because
> XACT_EVENT_PRE_COMMIT fires before the transaction is certain to
> commit - PreCommit_CheckForSerializationFailure or PreCommit_Notify
> could still error out.  We could have a hook that fires after that,
> but that doesn't solve the problem if a user of that hook can itself
> throw an error.  Even if part of the API contract is that it's not
> allowed to do so, the actual attempt to commit the change on the
> remote side can fail due to - e.g. - a network interruption, and
> that's go to be dealt with somehow.
>
>
Tom mentioned
--
in particular it treats the local transaction
asymmetrically from the remote ones, which doesn't seem like a great
idea --- ie, the local transaction could still abort after committing
all the remote ones, leaving you no better off in terms of cross-server
consistency.
--
You have given a specific example of this case. So, let me dry run through
CommitTransaction() after applying my patch.
1899 CallXactCallbacks(XACT_EVENT_PRE_COMMIT);

While processing this event in postgres_fdw's callback
pgfdw_xact_callback() sends a PREPARE TRANSACTION to all the foreign
servers involved. These servers return with their success or failures. Even
if one of them fails, the local transaction is aborted along-with all the
prepared transactions. Only if all the foreign servers succeed we proceed
further.

1925 PreCommit_CheckForSerializationFailure();
1926
1932 PreCommit_Notify();
1933

If any of these function (as you mentioned above), throws errors, the local
transaction will be aborted as well as the remote prepared transactions.
Note, that we haven't yet committed the local transaction (which will be
done below) and also not the remote transactions which are in PREPAREd
state there. Since all the transactions local as well as remote are aborted
in case of error, the data is still consistent. If these steps succeed, we
will proceed ahead.

1934 /* Prevent cancel/die interrupt while cleaning up */
1935 HOLD_INTERRUPTS();
1936
1937 /* Commit updates to the relation map --- do this as late as
possible */
1938 AtEOXact_RelationMap(true);
1939
1940 /*
1941  * set the current transaction state information appropriately
during
1942  * commit processing
1943  */
1944 s->state = TRANS_COMMIT;
1945
1946 /*
1947  * Here is where we really truly commit.
1948  */
1949 latestXid = RecordTransactionCommit();
1950
1951 TRACE_POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid);
1952
1953 /*
1954  * Let others know about no transaction in progress by me. Note
that this
1955  * must be done _before_ releasing locks we hold and _after_
1956  * RecordTransactionCommit.
1957  */
1958 ProcArrayEndTransaction(MyProc, latestXid);
1959

Local transaction committed. Remote transactions still in PREPAREd state.
Any server (including local) crash or link failure happens here, we leave
the remote transactions dangling in PREPAREd state and manual cleanup will
be required.

1975
1976 CallXactCallbacks(XACT_EVENT_COMMIT);

The postgresql callback pgfdw_xact_callback() commits the PREPAREd
transactions by sending COMMIT TRANSACTION to remote server (my patch). So,
I don't see why would my patch cause inconsistencies. It can cause dangling
PREPAREd transactions and I have already acknowledged that fact.

Am I missing something?



> --
> Robert Haas
> EnterpriseDB: http: