Re: [HACKERS] Strange Windows problem, lock_timeout test request

[Boszormenyi Zoltan]

2013-01-19 01:13 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:43 PM, Boszormenyi Zoltan wrote:

Hi,

using the MinGW cross-compiled PostgreSQL binaries from Fedora 18,
I get the following error for both 32 and 64-bit compiled executables.
listen_addresses = '*' and "trust" authentication was set for both.
The firewall was disabled for the tests and the server logs "incomplete startup 
packet".
The 64-bit version was compiled with my lock_timeout patch, the 32-bit
was without.

64-bit, connect to localhost:

C:\Users\Ákos\Desktop\PG93>bin\psql
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (127.0.0.1) and accepting
TCP/IP connections on port 5432?

64-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

32-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93-32>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

Does it ring a bell for someone?

Unfortunately, I won't have time to do anything with my lock_timeout patch
for about 3 weeks. Does anyone have a little spare time to test it on Windows?
The patch is here, it still applies to HEAD without rejects or fuzz:
http://www.postgresql.org/message-id/506c0854.7090...@cybertec.at


Yes it rings a bell. See 



I wanted to add a comment to this blog entry but it wasn't accepted.
Here it is:

It doesn't work under Wine, see:
http://www.winehq.org/pipermail/wine-users/2013-January/107008.html

But pg_config works so other PostgreSQL clients can also be built using the 
cross compiler.



Cross-compiling is not really a supported platform. Why don't you just build natively? 
This is know to work as shown by the buildfarm animals doing it successfully.


cheers

andrew







--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Strange Windows problem, lock_timeout test request

[Boszormenyi Zoltan]

2013-01-19 06:52 keltezéssel, Amit kapila írta:

On Saturday, January 19, 2013 4:13 AM Boszormenyi Zoltan wrote:

Hi,




Unfortunately, I won't have time to do anything with my lock_timeout patch
for about 3 weeks. Does anyone have a little spare time to test it on Windows?

I shall try to do it, probably next week.
Others are also welcome to test the patch.


Thanks very much in advance.

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Strange Windows problem, lock_timeout test request

[Boszormenyi Zoltan]

2013-01-19 01:13 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:43 PM, Boszormenyi Zoltan wrote:

Hi,

using the MinGW cross-compiled PostgreSQL binaries from Fedora 18,
I get the following error for both 32 and 64-bit compiled executables.
listen_addresses = '*' and "trust" authentication was set for both.
The firewall was disabled for the tests and the server logs "incomplete startup 
packet".
The 64-bit version was compiled with my lock_timeout patch, the 32-bit
was without.

64-bit, connect to localhost:

C:\Users\Ákos\Desktop\PG93>bin\psql
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (127.0.0.1) and accepting
TCP/IP connections on port 5432?

64-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

32-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93-32>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

Does it ring a bell for someone?

Unfortunately, I won't have time to do anything with my lock_timeout patch
for about 3 weeks. Does anyone have a little spare time to test it on Windows?
The patch is here, it still applies to HEAD without rejects or fuzz:
http://www.postgresql.org/message-id/506c0854.7090...@cybertec.at


Yes it rings a bell. See 



The strange thing is that I have used cross-compiled build
during Fedora 16 prime time (using the mingw-w64 test repository) and
early Fedora 17 using the mingw32-* and ming64-* GCC packages
that were pulled from the above mentioned repository. It was the last time
I tested PG on Windows and it worked. I could connect to it both locally
and from my Linux PC.



Cross-compiling is not really a supported platform. Why don't you just build natively? 
This is know to work as shown by the buildfarm animals doing it successfully.


Because I don't have a mingw setup on Windows. (Sorry.)

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Contrib PROGRAM problem

[Boszormenyi Zoltan]

2013-01-19 02:03 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 07:03 PM, Andrew Dunstan wrote:


It's not a good idea it seems.





Because that's only half of what I suggested.




This patch seems to do the right thing.

It probably needs to be applied to all the live branches.

cheers

andrew


Yes, this should fix the issue.

Thanks.

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Contrib PROGRAM problem

[Boszormenyi Zoltan]

2013-01-19 01:03 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:45 PM, Boszormenyi Zoltan wrote:

2013-01-18 23:37 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:19 PM, Boszormenyi Zoltan wrote:

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.






  ifdef PROGRAM
  $(PROGRAM): $(OBJS)
-$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@$(X)
  endif




Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and adjust the dependency 
for "all" in the same manner? Otherwise make will rebuild it whether or not it's 
needed, won't it?


With this in place:

all: $(PROGRAM)$(X) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) 
$(addsuffix .control, $(EXTENSION))


[zozo@localhost contrib]$ make
make -C adminpack all
make[1]: Entering directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make[1]: *** No rule to make target `.exe', needed by `all'. Stop.
make[1]: Leaving directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make: *** [all-adminpack-recurse] Error 2

It's not a good idea it seems.




Because that's only half of what I suggested.


No it's not. I only quoted half of the patch.



cheers

andrew







--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

--- src/makefiles/pgxs.mk.orig	2013-01-18 23:10:57.808370762 +0100
+++ src/makefiles/pgxs.mk	2013-01-18 23:44:46.180421400 +0100
@@ -100,7 +100,7 @@
 override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS)
 endif
 
-all: $(PROGRAM) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))
+all: $(PROGRAM)$(X) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))
 
 ifdef MODULE_big
 # shared library parameters
@@ -295,6 +295,6 @@
 endif
 
 ifdef PROGRAM
-$(PROGRAM): $(OBJS)
+$(PROGRAM)$(X): $(OBJS)
 	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
 endif

-- 
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] HS locking broken in HEAD

[Amit kapila]

On Friday, January 18, 2013 9:56 PM Andres Freund wrote:
On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
> Andres Freund  writes:
>>> > I am still stupefied nobody noticed that locking in HS (where just about
>>> > all locks are going to be fast path locks) was completely broken for
>>> > nearly two years.
>
>>> IIUC it would only be broken for cases where activity was going on
>>> concurrently in two different databases, which maybe isn't all that
>>> common out in the field.  And for sure it isn't something we test much.

>> I think effectively it only was broken in Hot Standby. At least I don't
>> immediately can think of a scenario where a strong lock is being acquired on 
>> a
>> non-shared object in a different database.

>> > I wonder if it'd be practical to, say, run all the contrib regression
>>> tests concurrently in different databases of one installation.

> I think it would be a good idea, but I don't immediately have an idea
> how to implement it. It seems to me we would need to put the logic for
> it into pg_regress? Otherwise the lifetime management of the shared
> postmaster seems to get complicated.

> What I would really like is to have some basic replication test scenario
> in the regression tests. That seems like a dramatically undertested, but
> pretty damn essential, part of the code.

> The first handwavy guess I have of doing it is something like connecting
> a second postmaster to the primary one at the start of the main
> regression tests (requires changing the wal level again, yuck) and
> fuzzyly comparing a pg_dump of the database remnants in both clusters at
> the end of the regression tests.

  I think this is good idea to start testing for replication. 
  In addition to this, I think to test secondary's behavior, there should be 
some kind of controller module
  which should control SQL commands run on  primary and secondary and match the 
expected behavior.
  This can be particulary useful to test locking conflicts and do the more 
specific tests.

With Regards,
Amit Kapila.

-- 
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: Proposal for Allow postgresql.conf values to be changed via SQL [review]

[Amit kapila]

On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>> Boszormenyi Zoltan  writes:
>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
> On using mktemp, linux compilation gives below warning
> warning: the use of `mktemp' is dangerous, better use `mkstemp'

> So I planned to use mkstemp.
 Good.
>>> On my HPUX box, the man page disapproves of both, calling them obsolete
>>> (and this man page is old enough to vote, I believe...)

>Then we have to at least consider what this old {p|s}age says... ;-)

>>
>>> Everywhere else that we need to do something like this, we just use our
>>> own PID to disambiguate, ie
>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>>> There is no need to deviate from that pattern or introduce portability
>>> issues, since we can reasonably assume that no non-Postgres programs are
>>> creating files in this directory.
>
>> Thanks for the enlightenment, I will post a new version soon.

> Here it is.

Thanks a ton for providing better modified version. 
I shall test it on Monday once and then we can conclude on it.

The only point in modifications, I am not comfortable is that in error messages 
you have 
mentioned (automatic configuration directory). I am not sure if we should use 
work "automatic"
for config_dir or just use configuration directory.
However I shall keep as it is if no one else has any objection. We can let 
committer decide about it.


With Regards,
Amit Kapila.



-- 
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] Strange Windows problem, lock_timeout test request

[Amit kapila]

On Saturday, January 19, 2013 4:13 AM Boszormenyi Zoltan wrote:
> Hi,



> Unfortunately, I won't have time to do anything with my lock_timeout patch
> for about 3 weeks. Does anyone have a little spare time to test it on Windows?

I shall try to do it, probably next week.
Others are also welcome to test the patch.


With Regards,
Amit Kapila.

-- 
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]Tablesample Submission

[Jaime Casanova]

On Thu, Jan 17, 2013 at 2:04 PM, Simon Riggs  wrote:
> On 17 January 2013 18:32, Josh Berkus  wrote:
>> On 11/04/2012 07:22 PM, Qi Huang wrote:
>>> Dear hackers Sorry for not replying the patch review. I didn't see the 
>>> review until recently as my mail box is full of Postgres mails and I didn't 
>>> notice the one for me, my mail box configuration problem. I am still 
>>> kind of busy with my university final year project. I shall not have time 
>>> to work on updating the patch until this semester finishes which is 
>>> December. I will work on then.Thanks.
>>> Best RegardsHuang Qi VictorComputer Science of National University of 
>>> Singapore
>>
>> Did you ever do the update of the patch?
>
> We aren't just waiting for a rebase, we're waiting for Hitoshi's
> comments to be addressed.
>
> I would add to them by saying I am very uncomfortable with the idea of
> allowing a TABLESAMPLE clause on an UPDATE or a DELETE. If you really
> want that you can use a sub-select.
>

also, i don't think that the REPEATABLE clause should be included in a
first revision.
and if we ever want to add more sample methods we can't just put
BERNOULLI nor SYSTEM in gram.y, a new catalog is probably needed
there.

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


-- 
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] Contrib PROGRAM problem

[Andrew Dunstan]

On 01/18/2013 11:59 PM, Peter Eisentraut wrote:

The above is the way it's done everywhere else in the source tree.

I think the reason this works is that either make or the system call
that make uses is aware of this naming issue somehow.



Oh, hmm, all these years playing with this stuff and I never realized 
msys make had these smarts built in, as it apparently does:


   $ cat xx.make
   foo:
touch foo.exe
   $ make -f xx.make
   touch foo.exe

   $ make -f xx.make
   make: `foo' is up to date.


Sorry for the noise.

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] Contrib PROGRAM problem

[Peter Eisentraut]

On Fri, 2013-01-18 at 17:37 -0500, Andrew Dunstan wrote:
> >   ifdef PROGRAM
> >   $(PROGRAM): $(OBJS)
> > - $(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX)
> $(LIBS) -o $@
> > + $(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX)
> $(LIBS) -o $@$(X)
> >   endif
> >
> 
> 
> Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and
> adjust 
> the dependency for "all" in the same manner? Otherwise make will
> rebuild 
> it whether or not it's needed, won't it?
> 
The above is the way it's done everywhere else in the source tree.

I think the reason this works is that either make or the system call
that make uses is aware of this naming issue somehow.




-- 
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] Contrib PROGRAM problem

[Andrew Dunstan]

On 01/18/2013 11:42 PM, Tom Lane wrote:

Andrew Dunstan  writes:

This patch seems to do the right thing.

Hmm ... seems kinda grotty ... isn't there a cleaner way?


It probably needs to be applied to all the live branches.

Agreed on back-patching once we have the right thing, but I don't like
this version too much.




I'm happy of you can find something cleaner.

We can't just have a dependency of all on $(PROGRAM)($X) because if 
PROGRAM is empty but X is not it will do something horrid like try to 
build ".exe".


If we do it the way Zoltan suggested then on Windows even if 
$(PROGRAM)$(X) exists and is up to date it could be rebuilt, since the 
target name won't exist.


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] Contrib PROGRAM problem

[Tom Lane]

Andrew Dunstan  writes:
> This patch seems to do the right thing.

Hmm ... seems kinda grotty ... isn't there a cleaner way?

> It probably needs to be applied to all the live branches.

Agreed on back-patching once we have the right thing, but I don't like
this version too much.

regards, tom lane


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

[HACKERS] dividing privileges for replication role.

[Tomonari Katsumata]

Hi,

I made a patch to divide privileges for replication role.

Currently(9.2), the privilege for replication role is
true / false which means that standby server is able to
connect to another server or not with the replication role.

This management and cascading replication make a strange behavior.
Because cascading replication is able to connect to another standby server,
we can see the cyclic situation.

This behavior has been discussed on Hackers-list(1),
but the conclusion was that's difficult to detect the situation.
(1) http://www.postgresql.org/message-id/50d12e8f.8000...@agliodbs.com

And then, I've reported a Bug-list(2) about this.
In this discussion, an idea that controlling
replication-connection with GUC parameter or privileges on
replication role comes up.
I think these can not avoid cyclic situation but will make some help for
DBA.
(2)
http://www.postgresql.org/message-id/e1ttvvj-0004b3...@wrigleys.postgresql.org


In this patch, I made below.
a) adding new privileges for replication:"MASTER REPLICATION" and "CASCADE
REPLICATION"
   "MASTER REPLICATION":  Replication-connection to master server is only
allowed
   "CASCADE REPLICATION": Replication-connection to cascade server is only
allowed
   ("REPLICATION" already implemented means replication-connection to both
servers is allowed)
b) addin above options in createuser command
   --master-replication
   --cascade-replication
c) dumping pg_authid.rolreplication value in pg_dumpall
   is changed by server version like this:
   from 9.1
 true  -> master-replication
 false -> noreplication
   from 9.2
 true  -> replication(master & cascade)
 false -> noreplication

I've not write any documents and tests for this yet,
but I want any comments whether this change is needed or not.

regards,
-
NTT Software Corporation
Tomonari Katsumata
*** a/src/backend/commands/user.c
--- b/src/backend/commands/user.c
***
*** 250,256  CreateRole(CreateRoleStmt *stmt)
if (dcanlogin)
canlogin = intVal(dcanlogin->arg) != 0;
if (disreplication)
!   isreplication = intVal(disreplication->arg) != 0;
if (dconnlimit)
{
connlimit = intVal(dconnlimit->arg);
--- 250,256 
if (dcanlogin)
canlogin = intVal(dcanlogin->arg) != 0;
if (disreplication)
!   isreplication = intVal(disreplication->arg);
if (dconnlimit)
{
connlimit = intVal(dconnlimit->arg);
***
*** 352,358  CreateRole(CreateRoleStmt *stmt)
/* superuser gets catupdate right by default */
new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);
new_record[Anum_pg_authid_rolcanlogin - 1] = BoolGetDatum(canlogin);
!   new_record[Anum_pg_authid_rolreplication - 1] = 
BoolGetDatum(isreplication);
new_record[Anum_pg_authid_rolconnlimit - 1] = Int32GetDatum(connlimit);
  
if (password)
--- 352,358 
/* superuser gets catupdate right by default */
new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);
new_record[Anum_pg_authid_rolcanlogin - 1] = BoolGetDatum(canlogin);
!   new_record[Anum_pg_authid_rolreplication - 1] = 
Int32GetDatum(isreplication);
new_record[Anum_pg_authid_rolconnlimit - 1] = Int32GetDatum(connlimit);
  
if (password)
***
*** 732,738  AlterRole(AlterRoleStmt *stmt)
  
if (isreplication >= 0)
{
!   new_record[Anum_pg_authid_rolreplication - 1] = 
BoolGetDatum(isreplication > 0);
new_record_repl[Anum_pg_authid_rolreplication - 1] = true;
}
  
--- 732,738 
  
if (isreplication >= 0)
{
!   new_record[Anum_pg_authid_rolreplication - 1] = 
Int32GetDatum(isreplication);
new_record_repl[Anum_pg_authid_rolreplication - 1] = true;
}
  
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 555,561  static void processCASbits(int cas_bits, int location, const 
char *constrType,
LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL
LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P
  
!   MAPPING MATCH MAXVALUE MINUTE_P MINVALUE MODE MONTH_P MOVE
  
NAME_P NAMES NATIONAL NATURAL NCHAR NEXT NO NONE
NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
--- 555,561 
LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL
LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P
  
!   MAPPING MASTER MATCH MAXVALUE MINUTE_P MINVALUE MODE MONTH_P MOVE
  
NAME_P NAMES NATIONAL NATURAL NCHAR NEXT NO NONE
NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
***
*** 571,577  static void processCASbits(int cas_bits, int location, const 
char *constrType,
QUOTE
  
RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REINDEX
!   RELATIVE_P RELEASE RENAME R

Re: [HACKERS] review: pgbench - aggregation of info written into log

[Tatsuo Ishii]

> On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii  wrote:
>> So if my understating is correct, 1)Tomas Vondra commits to work on
>> Windows support for 9.4, 2)on the assumption that one of Andrew
>> Dunstan, Dave Page or Magnus Hagander will help him in Windows
>> development.
>>
>> Ok? If so, I can commit the patch for 9.3 without Windows support. If
>> not, I will move the patch to next CF (for 9.4).
>>
>> Please correct me if I am wrong.
> 
> +1 for this approach.  I agree with Dave and Magnus that we don't want
> Windows to become a second-class platform, but this patch isn't making
> it so.  The #ifdef that peeks inside of an instr_time is already
> there, and it's not Tomas's fault that nobody has gotten around to
> fixing it before now.

Right.

> OTOH, I think that this sort of thing is quite wrong:
> 
> +#ifndef WIN32
> +"  --aggregate-interval NUM\n"
> +"   aggregate data over NUM seconds\n"
> +#endif
> 
> The right approach if this can't be supported on Windows is to still
> display the option in the --help output, and to display an error
> message if the user tries to use it, saying that it is not currently
> supported on Windows.  That fact should also be mentioned in the
> documentation.

Agreed. This seems to be much better approach.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp


-- 
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_ctl idempotent option

[Phil Sorber]

On Tue, Jan 15, 2013 at 11:06 AM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 1/14/13 10:22 AM, Tom Lane wrote:
>>> Also it appears to me that the hunk at lines 812ff is changing the
>>> default behavior, which is not what the patch is advertised to do.
>
>> True, I had forgotten to mention that.
>
>> Since it's already the behavior for start, another option would be to
>> just make it the default for stop as well and forget about the extra
>> options.  I'm not sure whether there is a big use case for getting an
>> error code on stop if the server is already stopped.
>
> Actually, I seem to recall having had to hack Red Hat's initscript
> because the LSB standard requires that stopping a not-running server
> *not* be an error.  So +1 for forgetting about the option entirely
> and just making it idempotent all the time.

+1

>
> 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


-- 
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] Contrib PROGRAM problem

[Andrew Dunstan]

On 01/18/2013 07:03 PM, Andrew Dunstan wrote:


It's not a good idea it seems.





Because that's only half of what I suggested.




This patch seems to do the right thing.

It probably needs to be applied to all the live branches.

cheers

andrew

diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 318d5ef..1d93d3e 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -100,7 +100,11 @@ ifdef PG_CPPFLAGS
 override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS)
 endif
 
-all: $(PROGRAM) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))
+ifdef PROGRAM
+PROGRAMX = $(PROGRAM)$(X)
+endif
+
+all: $(PROGRAMX) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))
 
 ifdef MODULE_big
 # shared library parameters
@@ -288,6 +292,6 @@ ifneq (,$(MODULES)$(MODULE_big))
 endif
 
 ifdef PROGRAM
-$(PROGRAM): $(OBJS)
+$(PROGRAMX): $(OBJS)
 	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
 endif

-- 
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] Strange Windows problem, lock_timeout test request

[Andrew Dunstan]

On 01/18/2013 05:43 PM, Boszormenyi Zoltan wrote:

Hi,

using the MinGW cross-compiled PostgreSQL binaries from Fedora 18,
I get the following error for both 32 and 64-bit compiled executables.
listen_addresses = '*' and "trust" authentication was set for both.
The firewall was disabled for the tests and the server logs 
"incomplete startup packet".

The 64-bit version was compiled with my lock_timeout patch, the 32-bit
was without.

64-bit, connect to localhost:

C:\Users\Ákos\Desktop\PG93>bin\psql
psql: could not connect to server: Operation would block 
(0x2733/10035)

Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (127.0.0.1) and 
accepting

TCP/IP connections on port 5432?

64-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block 
(0x2733/10035)

Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

32-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93-32>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block 
(0x2733/10035)

Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

Does it ring a bell for someone?

Unfortunately, I won't have time to do anything with my lock_timeout 
patch
for about 3 weeks. Does anyone have a little spare time to test it on 
Windows?

The patch is here, it still applies to HEAD without rejects or fuzz:
http://www.postgresql.org/message-id/506c0854.7090...@cybertec.at


Yes it rings a bell. See 



Cross-compiling is not really a supported platform. Why don't you just 
build natively? This is know to work as shown by the buildfarm animals 
doing it successfully.


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] Contrib PROGRAM problem

[Andrew Dunstan]

On 01/18/2013 05:45 PM, Boszormenyi Zoltan wrote:

2013-01-18 23:37 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:19 PM, Boszormenyi Zoltan wrote:

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.






  ifdef PROGRAM
  $(PROGRAM): $(OBJS)
-$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) 
$(LIBS) -o $@
+$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) 
$(LIBS) -o $@$(X)

  endif




Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and 
adjust the dependency for "all" in the same manner? Otherwise make 
will rebuild it whether or not it's needed, won't it?


With this in place:

all: $(PROGRAM)$(X) $(DATA_built) $(SCRIPTS_built) $(addsuffix 
$(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))


[zozo@localhost contrib]$ make
make -C adminpack all
make[1]: Entering directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make[1]: *** No rule to make target `.exe', needed by `all'. Stop.
make[1]: Leaving directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make: *** [all-adminpack-recurse] Error 2

It's not a good idea it seems.




Because that's only half of what I suggested.

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] My first patch! (to \df output)

[Phil Sorber]

On Sat, Dec 29, 2012 at 1:56 PM, Stephen Frost  wrote:
> * Jon Erdman (postgre...@thewickedtribe.net) wrote:
>> Oops! Here it is in the proper diff format. I didn't have my env set up 
>> correctly :(
>
> No biggie, and to get the bike-shedding started, I don't really like the
> column name or the values.. :)  I feel like something clearer would be
> "Runs_As" with "caller" or "owner"..  Saying "Security" makes me think
> of ACLs more than what user ID the function runs as, to be honest.
>
> Looking at the actual patch itself, it looks like you have some
> unecessary whitespace changes included..?
>
> Thanks!
>
> Stephen

Stephen, I think Jon's column name and values make a lot of sense.
That being said, I do agree with your point of making it clearer for
the person viewing the output, I just don't know if it would be
confusing when they wanted to change it or were trying to understand
how it related.

Agree on the extra spaces in the docs.

Jon, I think you inserted your changes improperly in the docs. The
classifications apply to the type, not to security.

Also, you need to use the %s place holder and the gettext_noop() call
for your values as well as your column name.

Compiles and tests ok. Results look as expected.


-- 
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] could not create directory "...": File exists

[Tom Lane]

Stephen Frost  writes:
> Tom,
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Don't see what.  The main reason we've not yet attempted a global fix is
>> that the most straightforward way (take a new snapshot each time we
>> start a new SnapshotNow scan) seems too expensive.  But CREATE DATABASE
>> is so expensive that the cost of an extra snapshot there ain't gonna
>> matter.

>   Patch attached.  Passes the regression tests and our internal testing
>   shows that it eliminates the error we were seeing (and doesn't cause
>   blocking, which is even better).

I committed this with a couple of changes:

* I used GetLatestSnapshot rather than GetTransactionSnapshot.  Since
we don't allow these operations inside transaction blocks, there
shouldn't be much difference, but in principle GetTransactionSnapshot
is the wrong thing; in a serializable transaction it could be quite old.

* After reviewing the other uses of SnapshotNow in dbcommands.c, I
decided we'd better make the same type of change in remove_dbtablespaces
and check_db_file_conflict, because those are likewise doing filesystem
operations on the strength of what they find in pg_tablespace.

I also ended up deciding to back-patch to 8.3 as well.

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] Contrib PROGRAM problem

[Boszormenyi Zoltan]

2013-01-18 23:37 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:19 PM, Boszormenyi Zoltan wrote:

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.






  ifdef PROGRAM
  $(PROGRAM): $(OBJS)
-$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@$(X)
  endif




Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and adjust the dependency 
for "all" in the same manner? Otherwise make will rebuild it whether or not it's needed, 
won't it?


With this in place:

all: $(PROGRAM)$(X) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) 
$(addsuffix .control, $(EXTENSION))


[zozo@localhost contrib]$ make
make -C adminpack all
make[1]: Entering directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make[1]: *** No rule to make target `.exe', needed by `all'.  Stop.
make[1]: Leaving directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make: *** [all-adminpack-recurse] Error 2

It's not a good idea it seems.




cheers

andrew







--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



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

[HACKERS] Strange Windows problem, lock_timeout test request

[Boszormenyi Zoltan]

Hi,

using the MinGW cross-compiled PostgreSQL binaries from Fedora 18,
I get the following error for both 32 and 64-bit compiled executables.
listen_addresses = '*' and "trust" authentication was set for both.
The firewall was disabled for the tests and the server logs "incomplete startup 
packet".
The 64-bit version was compiled with my lock_timeout patch, the 32-bit
was without.

64-bit, connect to localhost:

C:\Users\Ákos\Desktop\PG93>bin\psql
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "localhost" (127.0.0.1) and accepting
TCP/IP connections on port 5432?

64-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

32-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93-32>bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host "192.168.1.4" and accepting
TCP/IP connections on port 5432?

Does it ring a bell for someone?

Unfortunately, I won't have time to do anything with my lock_timeout patch
for about 3 weeks. Does anyone have a little spare time to test it on Windows?
The patch is here, it still applies to HEAD without rejects or fuzz:
http://www.postgresql.org/message-id/506c0854.7090...@cybertec.at

Thanks in advance,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Contrib PROGRAM problem

[Andrew Dunstan]

On 01/18/2013 05:19 PM, Boszormenyi Zoltan wrote:

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.






  ifdef PROGRAM
  $(PROGRAM): $(OBJS)
-   $(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@
+   $(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@$(X)
  endif




Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and adjust 
the dependency for "all" in the same manner? Otherwise make will rebuild 
it whether or not it's needed, won't it?



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] Event Triggers: adding information

[Robert Haas]

On Fri, Jan 18, 2013 at 5:12 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane  wrote:
>>> Well, that burden already exists for non-utility statements --- why
>>> should utility statements get a pass?  Other than that we tend to invent
>>> new utility syntax freely, which might be a good thing to discourage
>>> anyhow.
>
>> My concerns are that (1) it will slow down the addition of new
>> features to PostgreSQL by adding yet another barrier to commit and (2)
>> it won't be get enough use or regression test coverage to be, or
>> remain, bug-free.
>
> Meh.  The barriers to inventing new statements are already mighty tall.
> As for (2), I agree there's risk of bugs, but what alternative have you
> got that is likely to be less bug-prone?  At least a reverse-list
> capability could be tested standalone without having to set up a logical
> replication configuration.
>
> This should not be interpreted as saying I'm gung-ho to do this, mind
> you.  I'm just saying that if our intention is to support logical
> replication of all DDL operations, none of the alternatives look cheap.

Well, we agree on that, anyway.  :-)

I honestly don't know what to do about this.  I think you, Alvaro, and
I all have roughly the same opinion of this, which is that it doesn't
sound fun, but there's probably nothing better.  So, what do we do
when a really cool potential feature (logical replication of DDL)
butts up against an expensive future maintenance requirement?  I'm not
even sure what the criteria should be for making a decision on whether
it's "worth it".

-- 
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] Contrib PROGRAM problem

[Boszormenyi Zoltan]

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

--- src/makefiles/pgxs.mk.orig	2013-01-18 23:10:57.808370762 +0100
+++ src/makefiles/pgxs.mk	2013-01-18 23:11:22.741554459 +0100
@@ -296,5 +296,5 @@
 
 ifdef PROGRAM
 $(PROGRAM): $(OBJS)
-	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
 endif

-- 
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] Event Triggers: adding information

[Tom Lane]

Robert Haas  writes:
> On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane  wrote:
>> Well, that burden already exists for non-utility statements --- why
>> should utility statements get a pass?  Other than that we tend to invent
>> new utility syntax freely, which might be a good thing to discourage
>> anyhow.

> My concerns are that (1) it will slow down the addition of new
> features to PostgreSQL by adding yet another barrier to commit and (2)
> it won't be get enough use or regression test coverage to be, or
> remain, bug-free.

Meh.  The barriers to inventing new statements are already mighty tall.
As for (2), I agree there's risk of bugs, but what alternative have you
got that is likely to be less bug-prone?  At least a reverse-list
capability could be tested standalone without having to set up a logical
replication configuration.

This should not be interpreted as saying I'm gung-ho to do this, mind
you.  I'm just saying that if our intention is to support logical
replication of all DDL operations, none of the alternatives look cheap.

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] Contrib PROGRAM problem

[Alvaro Herrera]

Boszormenyi Zoltan wrote:


> I want to test my lock_timeout code under Windows and
> I compiled the whole PG universe with the MinGW cross-compiler
> for 64-bit under Fedora 18.
> 
> The problem contrib directories where Makefile contains
> PROGRAM = ...
> The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.

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


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

[HACKERS] Contrib PROGRAM problem

[Boszormenyi Zoltan]

Hi,

I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

[zozo@localhost oid2name]$ make
x86_64-w64-mingw32-gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
--param=ssp-buffer-size=4 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -I../../src/interfaces/libpq -I. 
-I. -I../../src/include -I./src/include/port/win32 -DEXEC_BACKEND 
"-I../../src/include/port/win32"  -c -o oid2name.o oid2name.c
x86_64-w64-mingw32-gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
--param=ssp-buffer-size=4 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard oid2name.o -L../../src/port 
-lpgport -L../../src/interfaces/libpq -lpq -L../../src/port 
-Wl,--allow-multiple-definition-lpgport -lz -lcrypt -lwsock32 -ldl -lm  -lws2_32 
-lshfolder -o oid2name


Note the "-o oid2name". Then "make install" of course fails, it expects
the .exe suffix:

[zozo@localhost oid2name]$ LANG=C make DESTDIR=/home/zozo/pgc93dev-win install
/usr/bin/mkdir -p 
'/home/zozo/pgc93dev-win/usr/x86_64-w64-mingw32/sys-root/mingw/bin'
/usr/bin/install -c  oid2name.exe 
'/home/zozo/pgc93dev-win/usr/x86_64-w64-mingw32/sys-root/mingw/bin'

/usr/bin/install: cannot stat 'oid2name.exe': No such file or directory
make: *** [install] Error 1

Ditto for "make clean":

[zozo@localhost oid2name]$ make clean
rm -f oid2name.exe
rm -f oid2name.o

All other binaries that are under src/bin or src/backend get
the proper .exe. DLLs are OK under both contrib or src.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Event Triggers: adding information

[Robert Haas]

On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Andres Freund escribió:
>>> Dimitri's earlier patches had support for quite some commands via
>>> deparsing and while its a noticeable amount of code it seemed to work
>>> ok.
>>> The last revision I could dig out is
>>> https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
>
>> I looked at that code back then and didn't like it very much.  The
>> problem I see (as Robert does, I think) is that it raises the burden
>> when you change the grammar -- you now need to edit not only gram.y, but
>> the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.
>
> Well, that burden already exists for non-utility statements --- why
> should utility statements get a pass?  Other than that we tend to invent
> new utility syntax freely, which might be a good thing to discourage
> anyhow.

My concerns are that (1) it will slow down the addition of new
features to PostgreSQL by adding yet another barrier to commit and (2)
it won't be get enough use or regression test coverage to be, or
remain, bug-free.

There may be other concerns with respect to the patch-as-proposed, but
those are my high-level concerns.

-- 
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] [WIP] pg_ping utility

[Phil Sorber]

On Tue, Dec 25, 2012 at 1:47 AM, Michael Paquier
 wrote:
>
>
> On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber  wrote:
>>
>> Updated patch attached.
>
> Thanks. I am marking this patch as ready for committer.
>
> --
> Michael Paquier
> http://michael.otacoo.com

Updated patch is rebased against current master and copyright year is updated.


pg_isready_bin_v10.diff
Description: Binary data


pg_isready_docs_v10.diff
Description: Binary 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] foreign key locks

[Andres Freund]

On 2013-01-18 16:01:18 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2013-01-18 15:37:47 -0500, Tom Lane wrote:
> >> I doubt it ever came up before.  What use is logging only the content of
> >> a buffer page?  Surely you'd need to know, for example, which relation
> >> and page number it is from.
> 
> > It only got to be a length of 0 because the the data got removed due to
> > a logged full page write. And the backup block contains the data about
> > which blocks it is logging in itself.
> 
> And if the full-page-image case *hadn't* been invoked, what then?  I
> still don't see a very good argument for xlog records with no fixed
> data.

In that case data would have been logged?

The code in question was:

xl_heap_lock_updated xlrec;
xlrec.target.node = rel->rd_node;
...
xlrec.xmax = new_xmax;

-   rdata.data = (char *) &xlrec;
-   rdata.len = SizeOfHeapLockUpdated;
-   rdata.buffer = buf;
-   rdata.buffer_std = true;
-   rdata.next = NULL;
-   recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED, &rdata);

Other wal logging code (and fklocks now as well) just put those into
two XLogRecData blocks to avoid the issue.

> > I wonder if the check shouldn't just check write_len instead, directly
> > below the loop that ads backup blocks.
> 
> We're not changing this unless you can convince me that the read-side
> error check mentioned in the comment is useless.

Youre right, the read side check is worth quite a bit. I think I am
retracting my suggestion.
I guess the amount of extra data thats uselessly logged although never
used in in the redo functions doesn't really amass to anything
significant in comparison to the backup block data.


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: Proposal for Allow postgresql.conf values to be changed via SQL [review]

[Boszormenyi Zoltan]

2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:

2013-01-18 21:32 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan  writes:

2013-01-18 11:05 keltezéssel, Amit kapila írta:

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.

Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)


Then we have to at least consider what this old {p|s}age says... ;-)



Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.


Thanks for the enlightenment, I will post a new version soon.


Here it is.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



set_persistent_v7.patch.gz
Description: Unix tar archive

-- 
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] foreign key locks

[Tom Lane]

Andres Freund  writes:
> On 2013-01-18 15:37:47 -0500, Tom Lane wrote:
>> I doubt it ever came up before.  What use is logging only the content of
>> a buffer page?  Surely you'd need to know, for example, which relation
>> and page number it is from.

> It only got to be a length of 0 because the the data got removed due to
> a logged full page write. And the backup block contains the data about
> which blocks it is logging in itself.

And if the full-page-image case *hadn't* been invoked, what then?  I
still don't see a very good argument for xlog records with no fixed
data.

> I wonder if the check shouldn't just check write_len instead, directly
> below the loop that ads backup blocks.

We're not changing this unless you can convince me that the read-side
error check mentioned in the comment is useless.

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: Proposal for Allow postgresql.conf values to be changed via SQL [review]

[Boszormenyi Zoltan]

2013-01-18 21:32 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan  writes:

2013-01-18 11:05 keltezéssel, Amit kapila írta:

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.

Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)

Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.


Thanks for the enlightenment, I will post a new version soon.



regards, tom lane





--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] foreign key locks

[Andres Freund]

On 2013-01-18 15:37:47 -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
> > The reason is that there is an (unknown to me) rule that there must be
> > some data not associated with a buffer:
> 
> > /*
> >  * NOTE: We disallow len == 0 because it provides a useful bit of extra
> >  * error checking in ReadRecord.  This means that all callers of
> >  * XLogInsert must supply at least some not-in-a-buffer data. [...]
> >  */
> 
> > This seems pretty strange to me.  And having the rule be spelled out
> > only in a comment within XLogInsert and not at its top, and not nearby
> > the XLogRecData struct definition either, seems pretty strange to me.
> > I wonder how come every PG hacker except me knows this.
> 
> I doubt it ever came up before.  What use is logging only the content of
> a buffer page?  Surely you'd need to know, for example, which relation
> and page number it is from.

It only got to be a length of 0 because the the data got removed due to
a logged full page write. And the backup block contains the data about
which blocks it is logging in itself.
I wonder if the check shouldn't just check write_len instead, directly
below the loop that ads backup blocks.

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] foreign key locks

[Tom Lane]

Alvaro Herrera  writes:
> The reason is that there is an (unknown to me) rule that there must be
> some data not associated with a buffer:

>   /*
>* NOTE: We disallow len == 0 because it provides a useful bit of extra
>* error checking in ReadRecord.  This means that all callers of
>* XLogInsert must supply at least some not-in-a-buffer data. [...]
>*/

> This seems pretty strange to me.  And having the rule be spelled out
> only in a comment within XLogInsert and not at its top, and not nearby
> the XLogRecData struct definition either, seems pretty strange to me.
> I wonder how come every PG hacker except me knows this.

I doubt it ever came up before.  What use is logging only the content of
a buffer page?  Surely you'd need to know, for example, which relation
and page number it is from.

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: Proposal for Allow postgresql.conf values to be changed via SQL [review]

[Tom Lane]

Boszormenyi Zoltan  writes:
> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>> On using mktemp, linux compilation gives below warning
>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>> 
>> So I planned to use mkstemp.

> Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)

Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.

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] foreign key locks

[Simon Riggs]

On 18 January 2013 20:02, Alvaro Herrera  wrote:
> XLOG_HEAP2_LOCK_UPDATED

Every xlog record needs to know where to put the block.

Compare with XLOG_HEAP_LOCK

xlrec.target.node = relation->rd_node;

-- 
 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] Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

[Boszormenyi Zoltan]

Hi,

comments below.

2013-01-18 11:05 keltezéssel, Amit kapila írta:

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.


Good.


In Windows, there is an api _mktemp_s, followed by open call, behaves in a 
similar way as mkstemp available in linux.
The code is changed to use of mkstemp functionality to generate a unique 
temporary file name instead of .lock file.
Please check this part of code, I am not very comfortable with using this API.


I read the documentation of _mktemp_s() at

http://msdn.microsoft.com/en-us/library/t8ex5e91%28v=vs.80%29.aspx

The comment says it's only for C++, but I can compile your patch using
the MinGW cross-compiler under Fedora 18. So either the MSDN comment
is false, or MinGW provides this function outside C++. More research is
needed on this.

However, I am not sure whether Cygwin provides the mkstemp() call or not.
Searching... Found bugzilla reports against mkstemp on Cygwin. So, yes, it does
and your code would clash with it. In port/dirmod.c:

8<8<8<8<8<
#if defined(WIN32) || defined(__CYGWIN__)

...

/*
 *  pgmkstemp
 */
int
pgmkstemp (char *TmpFileName)
{
int err;

err = _mktemp_s(TmpFileName, strlen(TmpFileName) + 1);
if (err != 0)
return -1;

return open(TmpFileName, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR);
}

/* We undefined these above; now redefine for possible use below */
#define rename(from, to)pgrename(from, to)
#define unlink(path)pgunlink(path)
#define mkstemp(tempfilename)   pgmkstemp(tempfilename)
#endif   /* defined(WIN32) || defined(__CYGWIN__) */
8<8<8<8<8<

pgmkstemp() should be defined in its own block inside

#if defined(WIN32) && !defined(__CYGWIN__)
...
#endif

Same thing applies to src/include/port.h:

8<8<8<8<8<
#if defined(WIN32) || defined(__CYGWIN__)
/*
 *  Win32 doesn't have reliable rename/unlink during concurrent access.
 */
extern int  pgrename(const char *from, const char *to);
extern int  pgunlink(const char *path);
extern int pgmkstemp (char *TmpFileName);

/* Include this first so later includes don't see these defines */
#ifdef WIN32_ONLY_COMPILER
#include 
#endif

#define rename(from, to)pgrename(from, to)
#define unlink(path)pgunlink(path)
#define mkstemp(tempfilename)   pgmkstemp(tempfilename)
#endif   /* defined(WIN32) || defined(__CYGWIN__) */
8<8<8<8<8<


Removed the code which is used for deleting the .lock file in case of backend 
crash or during server startup.


Good.


Added a new function RemoveAutoConfTmpFiles used for deleting the temp files 
left out any during previous
postmaster session in the server startup.


Good.


In SendDir(), used sizeof() -1


Good.

Since you have these defined:

+ #define PG_AUTOCONF_DIR "config_dir"
+ #define PG_AUTOCONF_FILENAME "postgresql.auto.conf"
+ #define PG_AUTOCONF_SAMPLE_TMPFILE "postgresql.auto.conf.XX"
+ #define PG_AUTOCONF_TMP_FILE"postgresql.auto.conf."

then the hardcoded values can also be changed everywhere. But to kill
them in initdb.c, these #define's must be in pg_config_manual.h instead of
guc.h.

Most notably, postgresql.conf.sample contains:
#include_dir = 'auto.conf.d'
and initdb replaces it with
include_dir = 'config_dir'.
I prefer the auto.conf.d directory name. After killing all hardcoded
"config_dir", changing one #define is all it takes to change it.

There are two blocks of code that Tom commented as being "over-engineered":

+   /* Frame auto conf name and auto conf sample temp file name */
+   snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR, 
PG_AUTOCONF_FILENAME);
+   StrNCpy(AutoConfFileName, ConfigFileName, sizeof(AutoConfFileName));
+   get_parent_directory(AutoConfFileName);
+   join_path_components(AutoConfFileName, AutoConfFileName, Filename);
+   canonicalize_path(AutoConfFileName);
+
+   snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR, 
PG_AUTOCONF_SAMPLE_TMPFILE);

+   StrNCpy(AutoConfTmpFileName, ConfigFileName, 
sizeof(AutoConfTmpFileName));
+   get_parent_directory(AutoConfTmpFileName);
+   join_path_components(AutoConfTmpFileName, AutoConfTmpFileName, 
Filename);
+   canonicalize_path(AutoConfTmpFileName);

You can simply do
snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_FILENAME);
snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName),"%s/%s/%s",
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_SAMPL

Re: [HACKERS] foreign key locks

[Alvaro Herrera]

Alvaro Herrera wrote:
> Here's version 28 of this patch.  The only substantive change here from
> v26 is that I've made GetTupleForTrigger() use either LockTupleExclusive
> or LockTupleNoKeyExclusive, depending on whether the key columns are
> being modified by the update.  This needs to go through EvalPlanQual, so
> that function is now getting the lock mode as a parameter instead of
> hardcoded LockTupleExclusive.  (All other users of GetTupleForTrigger
> still use LockTupleExclusive, so there's no change for anybody other
> than FOR EACH ROW BEFORE UPDATE triggers).
> 
> At this point, I think I've done all I wanted to do in connection with
> this patch, and I intend to commit.  I think this is a good time to get
> it committed, so that people can play with it more extensively and
> report any breakage I might have caused before we even get into beta.

While trying this out this morning I noticed a bug in the XLog code:
trying to lock the updated version of a tuple when the page that
contains the updated version requires a backup block, would cause this:

PANIC:  invalid xlog record length 0

The reason is that there is an (unknown to me) rule that there must be
some data not associated with a buffer:

/*
 * NOTE: We disallow len == 0 because it provides a useful bit of extra
 * error checking in ReadRecord.  This means that all callers of
 * XLogInsert must supply at least some not-in-a-buffer data. [...]
 */

This seems pretty strange to me.  And having the rule be spelled out
only in a comment within XLogInsert and not at its top, and not nearby
the XLogRecData struct definition either, seems pretty strange to me.
I wonder how come every PG hacker except me knows this.

For the curious, I attach an isolationtester spec file I used to
reproduce the problem (and verify the fix).

To fix the crash I needed to do this:

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 99fced1..9762890 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4838,7 +4838,7 @@ l4:
{
xl_heap_lock_updated xlrec;
XLogRecPtr  recptr;
-   XLogRecData rdata;
+   XLogRecData rdata[2];
Pagepage = BufferGetPage(buf);
 
xlrec.target.node = rel->rd_node;
@@ -4846,13 +4846,18 @@ l4:
xlrec.xmax = new_xmax;
xlrec.infobits_set = compute_infobits(new_infomask, 
new_infomask2);
 
-   rdata.data = (char *) &xlrec;
-   rdata.len = SizeOfHeapLockUpdated;
-   rdata.buffer = buf;
-   rdata.buffer_std = true;
-   rdata.next = NULL;
+   rdata[0].data = (char *) &xlrec;
+   rdata[0].len = SizeOfHeapLockUpdated;
+   rdata[0].buffer = InvalidBuffer;
+   rdata[0].next = &(rdata[1]);
+
+   rdata[1].data = NULL;
+   rdata[1].len = 0;
+   rdata[1].buffer = buf;
+   rdata[1].buffer_std = true;
+   rdata[1].next = NULL;
 
-   recptr = XLogInsert(RM_HEAP2_ID, 
XLOG_HEAP2_LOCK_UPDATED, &rdata);
+   recptr = XLogInsert(RM_HEAP2_ID, 
XLOG_HEAP2_LOCK_UPDATED, rdata);
 
PageSetLSN(page, recptr);
PageSetTLI(page, ThisTimeLineID);

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
setup
{
  CREATE TABLE foo (
key serial PRIMARY KEY,
value   int
  );

  INSERT INTO foo SELECT value FROM generate_series(1, 226) AS value;
}

teardown
{
  DROP TABLE foo;
}

session "s1"
step "s1b"  { BEGIN ISOLATION LEVEL REPEATABLE READ; }
step "s1s"  { SELECT * FROM foo LIMIT 0; }  # obtain snapshot
step "s1l"  { SELECT * FROM foo WHERE key = 1 FOR KEY SHARE; } # obtain lock
step "s1c"  { COMMIT; }

session "s2"
step "s2b"  { BEGIN; }
step "s2u"  { UPDATE foo SET value = 2 WHERE key = 1; }
step "s2c"  { COMMIT; }

session "s3"
step "s3ck" { CHECKPOINT; }

permutation "s1b" "s1s" "s2b" "s2u" "s3ck" "s1l" "s1c" "s2c"

-- 
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] could not create directory "...": File exists

[Tom Lane]

Stephen Frost  writes:
>   Patch attached.  Passes the regression tests and our internal testing
>   shows that it eliminates the error we were seeing (and doesn't cause
>   blocking, which is even better).

>   We have a workaround in place for our build system (more-or-less
>   "don't do that" approach), but it'd really be great if this was
>   back-patched and in the next round of point releases.  Glancing
>   through the branches, looks like it should apply pretty cleanly.

It looks like it will work back to 8.4; before that we didn't have
RegisterSnapshot.  The patch could be adjusted for 8.3 if anyone
is sufficiently excited about it, but personally I'm not.

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] in-catalog Extension Scripts and Control parameters (templates?)

[Stephen Frost]

* Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
> Tom Lane  writes:
> > We already do: see text search templates.  The code tends to call those
> > TSTEMPLATEs, so I'd suggest ACL_KIND_EXTTEMPLATE or some such.  I agree
> > with Stephen's objection to use of the bare word "template".
> 
> Yes, me too, but I had a hard time to convince myself of using such a
> wordy notation. I will adjust the patch. Is that all I have to adjust
> before finishing the command set support? :)

I'm keeping a healthy distance away from *that*.. ;)

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)

[Dimitri Fontaine]

Tom Lane  writes:
> We already do: see text search templates.  The code tends to call those
> TSTEMPLATEs, so I'd suggest ACL_KIND_EXTTEMPLATE or some such.  I agree
> with Stephen's objection to use of the bare word "template".

Yes, me too, but I had a hard time to convince myself of using such a
wordy notation. I will adjust the patch. Is that all I have to adjust
before finishing the command set support? :)

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] Event Triggers: adding information

[Tom Lane]

Alvaro Herrera  writes:
> Andres Freund escribió:
>> Dimitri's earlier patches had support for quite some commands via
>> deparsing and while its a noticeable amount of code it seemed to work
>> ok.
>> The last revision I could dig out is
>> https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

> I looked at that code back then and didn't like it very much.  The
> problem I see (as Robert does, I think) is that it raises the burden
> when you change the grammar -- you now need to edit not only gram.y, but
> the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

Well, that burden already exists for non-utility statements --- why
should utility statements get a pass?  Other than that we tend to invent
new utility syntax freely, which might be a good thing to discourage
anyhow.

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] missing rename support

[Tom Lane]

Stephen Frost  writes:
> * Ali Dar (ali.munir@gmail.com) wrote:
>> Find attached an initial patch for ALTER RENAME RULE feature. Please
>> note that it does not have any documentation yet.

> Just took a quick look through this.  Seems to be alright, but why do we
> allow renaming ON SELECT rules at all, given that they must be named
> _RETURN?  My thinking would be to check for that case and error out if
> someone tries it.

Agreed, we should exclude ON SELECT rules.

> You should try to keep variables lined up:

pgindent is probably a better answer than trying to get this right
manually.

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] in-catalog Extension Scripts and Control parameters (templates?)

[Tom Lane]

Stephen Frost  writes:
> 'Extension Template' is fine, I was just objecting to places in the code
> where it just says 'TEMPLATE'.  I imagine we might have some 'XXX
> Template' at some point in the future and then we'd have confusion
> between "is this an *extension* template or an XXX template?".

We already do: see text search templates.  The code tends to call those
TSTEMPLATEs, so I'd suggest ACL_KIND_EXTTEMPLATE or some such.  I agree
with Stephen's objection to use of the bare word "template".

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] Event Triggers: adding information

[Andres Freund]

On 2013-01-18 15:22:55 -0300, Alvaro Herrera wrote:
> Andres Freund escribió:
> > On 2013-01-18 12:44:13 -0500, Tom Lane wrote:
> 
> > > An issue that would have to be thought about is that there are assorted
> > > scenarios where we generate "parse trees" that contain options that
> > > cannot be generated from SQL, so there's no way to reverse-list them
> > > into working SQL.  But often those hidden options are essential to know
> > > what the statement will really do, so I'm not sure ignoring them will be
> > > a workable answer for replication purposes.
> > 
> > Dimitri's earlier patches had support for quite some commands via
> > deparsing and while its a noticeable amount of code it seemed to work
> > ok.
> > The last revision I could dig out is
> > https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
> > 
> > I think some things in there would have to change (e.g. I doubt that its
> > good that EventTriggerData is involved at that level) but I think it
> > shows very roughly how it could look.
> 
> I looked at that code back then and didn't like it very much.  The
> problem I see (as Robert does, I think) is that it raises the burden
> when you change the grammar -- you now need to edit not only gram.y, but
> the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

Yea, but thats nothing new, you already need to do all that for normal
SQL. Changes that to the grammar that don't involve modifying ruleutils.c et
al. are mostly trivial enough that this doesn't really place that much of a
burden.

And I yet to hear any other proposal of how to do this?

> Another problem is what Tom mentions: there are internal options that
> cannot readily be turned back into SQL.  We would have to expose these
> at the SQL level somehow; but to me that looks painful because we would
> be offering users the option to break their systems by calling commands
> that do things that should only be done internally.

Hm. Which options are you two thinking of right now?

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] 9.3 Pre-proposal: Range Merge Join

[Jeff Davis]

On Fri, 2013-01-18 at 12:25 +0100, Stefan Keller wrote:
> Sounds good.
> Did you already had contact e.g. with Paul (cc'ed just in case)?
> And will this clever index also be available within all these hundreds
> of PostGIS functions?

Yes, I've brought the idea up to Paul before, but thank you.

It's not an index exactly, it's a new join algorithm that should be more
efficient for spatial joins. That being said, it's not done, so it could
be an index by the time I'm finished ;)

When it is done, it will probably need some minor work in PostGIS to
make use of it. But that work is done at the datatype level, and PostGIS
only has a couple data types, so I don't think it will be a lot of work.

Regards,
Jeff Davis



-- 
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] Event Triggers: adding information

[Alvaro Herrera]

Andres Freund escribió:
> On 2013-01-18 12:44:13 -0500, Tom Lane wrote:

> > An issue that would have to be thought about is that there are assorted
> > scenarios where we generate "parse trees" that contain options that
> > cannot be generated from SQL, so there's no way to reverse-list them
> > into working SQL.  But often those hidden options are essential to know
> > what the statement will really do, so I'm not sure ignoring them will be
> > a workable answer for replication purposes.
> 
> Dimitri's earlier patches had support for quite some commands via
> deparsing and while its a noticeable amount of code it seemed to work
> ok.
> The last revision I could dig out is
> https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
> 
> I think some things in there would have to change (e.g. I doubt that its
> good that EventTriggerData is involved at that level) but I think it
> shows very roughly how it could look.

I looked at that code back then and didn't like it very much.  The
problem I see (as Robert does, I think) is that it raises the burden
when you change the grammar -- you now need to edit not only gram.y, but
the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.
One idea I had was to add annotations on gram.y so that an external
(Perl?) program could generate the ddl_rewrite.c equivalent, without
forcing the developer to do it by hand.

Another problem is what Tom mentions: there are internal options that
cannot readily be turned back into SQL.  We would have to expose these
at the SQL level somehow; but to me that looks painful because we would
be offering users the option to break their systems by calling commands
that do things that should only be done internally.

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


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

Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)

[Stephen Frost]

* Andres Freund (and...@2ndquadrant.com) wrote:
> On 2013-01-18 12:45:02 -0500, Stephen Frost wrote:
> > 'Template' seems like a really broad term which might end up being
> > associated with things beyond extensions, yet there are a number of
> > places where you just use 'TEMPLATE', eg, ACL_KIND_TEMPLATE.  Seems like
> > it might become an issue later.
> 
> I think Tom came up with that name and while several people (including
> me and I think also Dim) didn't really like it nobody has come up with a
> better name so far.

'Extension Template' is fine, I was just objecting to places in the code
where it just says 'TEMPLATE'.  I imagine we might have some 'XXX
Template' at some point in the future and then we'd have confusion
between "is this an *extension* template or an XXX template?".

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] missing rename support

[Stephen Frost]

* Ali Dar (ali.munir@gmail.com) wrote:
> Find attached an initial patch for ALTER RENAME RULE feature. Please
> note that it does not have any documentation yet.

Just took a quick look through this.  Seems to be alright, but why do we
allow renaming ON SELECT rules at all, given that they must be named
_RETURN?  My thinking would be to check for that case and error out if
someone tries it.

You should try to keep variables lined up:

Relationpg_rewrite_desc;
HeapTuple   ruletup;
+   Oid owningRel;

should be:

Relationpg_rewrite_desc;
HeapTuple   ruletup;
+   Oid owningRel;

I'd also strongly recommend looking through that entire function very
carefully.  Code that's been #ifdef'd out tends to rot.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)

[Andres Freund]

On 2013-01-18 12:45:02 -0500, Stephen Frost wrote:
> * Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
> > Please find attached a preliminary patch following the TEMPLATE ideas,
> > and thanks in particular to Tom and Heikki for a practical design about
> > how to solve that problem!
> 
> Given that it's preliminary and v0 and big and whatnot, it seems like
> it should be bounced to post-9.3.  Even so, I did take a look through
> it, probably mostly because I'd really like to see this feature. :)

To be fair, the patch start its life pretty early on in the cycle and
only got really reviewed (and I think updated) later. I just got
rewritten into this form based on review.

> 'Template' seems like a really broad term which might end up being
> associated with things beyond extensions, yet there are a number of
> places where you just use 'TEMPLATE', eg, ACL_KIND_TEMPLATE.  Seems like
> it might become an issue later.

I think Tom came up with that name and while several people (including
me and I think also Dim) didn't really like it nobody has come up with a
better name so far.

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] Inconsistent format() behavior for argument-count inconsistency

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Why do we throw an error for too few arguments, but not too many?

Not sure offhand, though I could see how it might be useful.  A use-case
might be that you have a variable template string which is user defined,
where they can choose from the arguments that are passed which ones they
want displayed and how.  Otherwise, you'd have to have a bunch of
different call sites to format() depending on which options are
requested.

I'd go look at what C sprintf() does, as I feel like that's what we're
trying to emulate and see what it does.

There is also a patch that I reviewed/commented on about changing format
around a bit.  I'm guessing it needs more review/work, just havn't
gotten back around to it yet.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Event Triggers: adding information

[Andres Freund]

On 2013-01-18 12:44:13 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
> >> I'm having trouble following this.  Can you restate?  I wasn't sure
> >> what you meant by libpqdump.  I assumed you were speaking of a
> >> parsetree->DDL or catalog->DDL facility.
>
> > Yea, that wasn't really clear, sorry for that.
>
> > What I was trying to say that I think doing parstree->text conversion
> > out of tree is noticeably more complex and essentially places a higher
> > maintenance burden on the project because
>
> Ah.  That's not pg_dump's job though, so you're choosing a bad name for
> it.

I really only mentioned libpgdump because the object access hooks at the
moment only get passed the object oid and because Robert doubted the
need for deparsing the parsetrees in the past. Without the parsetree you
obviousl cannot deparse the parsetree ;)

I do *not* think that using something that reassembles the statement
solely based on the catalog context is a good idea.

> What I think you are really talking about is extending the
> deparsing logic in ruleutils.c to cover utility statements.  Which is
> not a bad plan in principle; we've just never needed it before.  It
> would be quite a lot of new code though.

Yes, I think that is the only realistic approach at providing somewhat
unambigious SQL to replicate DDL.

> An issue that would have to be thought about is that there are assorted
> scenarios where we generate "parse trees" that contain options that
> cannot be generated from SQL, so there's no way to reverse-list them
> into working SQL.  But often those hidden options are essential to know
> what the statement will really do, so I'm not sure ignoring them will be
> a workable answer for replication purposes.

Dimitri's earlier patches had support for quite some commands via
deparsing and while its a noticeable amount of code it seemed to work
ok.
The last revision I could dig out is
https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

I think some things in there would have to change (e.g. I doubt that its
good that EventTriggerData is involved at that level) but I think it
shows very roughly how it could look.

Greetings,

Andres

--
 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

[HACKERS] Inconsistent format() behavior for argument-count inconsistency

[Tom Lane]

regression=# select format('%s %s', 'foo', 'bar');
 format  
-
 foo bar
(1 row)

regression=# select format('%s %s', 'foo', 'bar', 'baz');
 format  
-
 foo bar
(1 row)

regression=# select format('%s %s', 'foo');  
ERROR:  too few arguments for format

Why do we throw an error for too few arguments, but not too many?

(This came up when I started wondering whether the proposed VARIADIC
feature would really be very useful for format(), since it needs a
format string that matches up with its arguments.)

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] in-catalog Extension Scripts and Control parameters (templates?)

[Stephen Frost]

* Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
> Please find attached a preliminary patch following the TEMPLATE ideas,
> and thanks in particular to Tom and Heikki for a practical design about
> how to solve that problem!

Given that it's preliminary and v0 and big and whatnot, it seems like
it should be bounced to post-9.3.  Even so, I did take a look through
it, probably mostly because I'd really like to see this feature. :)

What's with removing the OBJECT_TABLESPACE case?  Given that
get_object_address_tmpl() seems to mainly just fall into a couple of
case statements to split out the different options, I'm not sure that
having that function is useful, or perhaps it should be 2 distinct
functions?

ExtensionControlFile seemed like a good name, just changing that to
"ExtensionControl" doesn't seem as nice, tho that's a bit of bike
shedding, I suppose.

I'm not sure we have a 'dile system'... :)

For my 2c, I wish we could do something better than having to support
both on-disk conf files and in-database configs.  Don't have any
particular solution to that tho.

Also pretty sure we only have one catalog
('get_ext_ver_list_from_catalogs')

'Template' seems like a really broad term which might end up being
associated with things beyond extensions, yet there are a number of
places where you just use 'TEMPLATE', eg, ACL_KIND_TEMPLATE.  Seems like
it might become an issue later.

Just a side-note, there's also some whitespace issues.

Also, no pg_dump/restore support..?  Seems like that'd be useful..

That's just a real quick run-through with my notes.  If this patch is
really gonna go into 9.3, I'll try to take a deeper look.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Event Triggers: adding information

[Tom Lane]

Andres Freund  writes:
> On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
>> I'm having trouble following this.  Can you restate?  I wasn't sure
>> what you meant by libpqdump.  I assumed you were speaking of a
>> parsetree->DDL or catalog->DDL facility.

> Yea, that wasn't really clear, sorry for that.

> What I was trying to say that I think doing parstree->text conversion
> out of tree is noticeably more complex and essentially places a higher
> maintenance burden on the project because

Ah.  That's not pg_dump's job though, so you're choosing a bad name for
it.  What I think you are really talking about is extending the
deparsing logic in ruleutils.c to cover utility statements.  Which is
not a bad plan in principle; we've just never needed it before.  It
would be quite a lot of new code though.

An issue that would have to be thought about is that there are assorted
scenarios where we generate "parse trees" that contain options that
cannot be generated from SQL, so there's no way to reverse-list them
into working SQL.  But often those hidden options are essential to know
what the statement will really do, so I'm not sure ignoring them will be
a workable answer for replication purposes.

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] logical changeset generation v4

[Tom Lane]

Robert Haas  writes:
> I took a quick look at this and am just curious why we're adding the
> requirement that t_tableOid has to be initialized?

I assume he meant it had been left at a random value, which is surely
bad practice even if a specific usage doesn't fall over today.

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] logical changeset generation v4

[Andres Freund]

On 2013-01-18 11:48:43 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 11:33 AM, Alvaro Herrera
>  wrote:
> > Andres Freund wrote:
> >
> >> [09] Adjust all *Satisfies routines to take a HeapTuple instead of a 
> >> HeapTupleHeader
> >>
> >> For timetravel access to the catalog we need to be able to lookup (cmin,
> >> cmax) pairs of catalog rows when were 'inside' that TX. This patch just
> >> adapts the signature of the *Satisfies routines to expect a HeapTuple
> >> instead of a HeapTupleHeader. The amount of changes for that is fairly
> >> low as the HeapTupleSatisfiesVisibility macro already expected the
> >> former.
> >>
> >> It also makes sure the HeapTuple fields are setup in the few places that
> >> didn't already do so.
> >
> > I had a look at this part.  Running the regression tests unveiled a case
> > where the tableOid wasn't being set (and thus caused an assertion to
> > fail), so I added that.  I also noticed that the additions to
> > pruneheap.c are sometimes filling a tuple before it's strictly
> > necessary, leading to wasted work.  Moved those too.
> >
> > Looks good to me as attached.
>
> I took a quick look at this and am just curious why we're adding the
> requirement that t_tableOid has to be initialized?

Its a stepping stone for catalog timetravel. I separated it into a different
patch because it seems to make the real patch easier to review without having
to deal with all those unrelated hunks.

The reason why we need t_tableOid and a valid ItemPointer is that during
catalog timetravel (so we can decode the heaptuples in WAL) we need to
see tuples in the catalog that have been changed in the transaction we
travelled to. That means we need to lookup cmin/cmax values which aren't
stored separately anymore.

My first approach was to build support for logging allocated combocids
(only for catalog tables) and use the existing combocid infrastructure
to look them up.
Turns out thats not a correct solution, consider this:
* T100: INSERT (xmin: 100, xmax: Invalid, (cmin|cmax): 3)
* T101: UPDATE (xmin: 100, xmax: 101, (cmin|cmax): 10)

If you know travel to T100 and you want to decide whether that tuple is
visible when in CommandId = 5 you have the problem that the original
cmin value has been overwritten by the cmax from T101. Note that in this
scenario no ComboCids have been generated!
The problematic part is that the information about what happened is
only available in T101.

I took resolve to doing something similar to what the heap rewrite code
uses to track update chains. Everytime a catalog tuple
inserted/updated/deleted (filenode, ctid, cmin, cmax) is wal logged (if
wal_level=logical) and while traveling to a transaction all those are
put up in a hash table so they can get looked up if we need the
respective cmin/cmax values. As we do that for all modifications of
catalog tuples in that transaction we only ever need that mapping when
inspecting that specific transaction.

Seems to work very nicely, I have made quite some tests with it and I
know of no failure cases.


To be able to make that lookup we need to get the relfilenode & item
pointer of the tuple were just looking up. Thats why I changed the
signature to pass a HeapTuple instead of a HeapTupleHeader. We get the
relfilenode from the buffer that has been passed *not* from the passed
table oid.
So requiring a valid table oid isn't strictly required as long as the
item pointer is valid, but it has made debugging noticeably easier.

Makes sense?

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] HS locking broken in HEAD

[Tom Lane]

Andres Freund  writes:
> On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
>> I wonder if it'd be practical to, say, run all the contrib regression
>> tests concurrently in different databases of one installation.

> I think it would be a good idea, but I don't immediately have an idea
> how to implement it. It seems to me we would need to put the logic for
> it into pg_regress? Otherwise the lifetime management of the shared
> postmaster seems to get complicated.

Seems like it'd be sufficient to make it work for the "make
installcheck" case, which dodges the postmaster problem.

> What I would really like is to have some basic replication test scenario
> in the regression tests.

Agreed, that's not being tested enough either.

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] review: pgbench - aggregation of info written into log

[Robert Haas]

On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii  wrote:
> So if my understating is correct, 1)Tomas Vondra commits to work on
> Windows support for 9.4, 2)on the assumption that one of Andrew
> Dunstan, Dave Page or Magnus Hagander will help him in Windows
> development.
>
> Ok? If so, I can commit the patch for 9.3 without Windows support. If
> not, I will move the patch to next CF (for 9.4).
>
> Please correct me if I am wrong.

+1 for this approach.  I agree with Dave and Magnus that we don't want
Windows to become a second-class platform, but this patch isn't making
it so.  The #ifdef that peeks inside of an instr_time is already
there, and it's not Tomas's fault that nobody has gotten around to
fixing it before now.

OTOH, I think that this sort of thing is quite wrong:

+#ifndef WIN32
+  "  --aggregate-interval NUM\n"
+  "   aggregate data over NUM seconds\n"
+#endif

The right approach if this can't be supported on Windows is to still
display the option in the --help output, and to display an error
message if the user tries to use it, saying that it is not currently
supported on Windows.  That fact should also be mentioned in the
documentation.

-- 
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] Event Triggers: adding information

[Andres Freund]

On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund  
> wrote:
> >> No, there's one also in heap_create_with_catalog.  Took me a minute to
> >> find it, as it does not use InvokeObjectAccessHook.  The idea is that
> >> OAT_POST_CREATE fires once per object creation, regardless of the
> >> object type - table, column, whatever.
> >
> > Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
> > comment explaining why we InvokeObjectAccessHook isn't used just for
> > enhanced grepability.
> 
> I cannot parse that sentence, but I agree that the ungreppability of
> it is suboptimal.  I resorted to git log -SInvokeObjectAccessHook -p
> to find it.

I was thinking of adding a comment like
/* We don't use InvokeObjectAccessHook here because we need to ... */
not because the comment in itself is important but because it allows to
grep ;)


> >> I have been involved in PostgreSQL development for about 4.5 years
> >> now.  This is less time than many people here, but it's still long
> >> enough to say a whole lot of people ask for some variant of this idea,
> >> and yet I have yet to see anybody produce a complete, working version
> >> of this functionality and maintain it outside of the PostgreSQL tree
> >> for one release cycle (did I miss something?).
> >
> > I don't really think thats a fair argument.
> >
> > For one, I didn't really ask for a libpgdump - I said that I don't see
> > any way to generate re-executable SQL without it if we don't get the
> > parse-tree but only the oid of the created object. Not to speak of the
> > complexities of supporting ALTER that way (which is, as you note,
> > basically not the way to do this).
> 
> Oh, OK.  I see.  Well, I've got no problem making the parse tree
> available via InvokeObjectAccessHook.  Isn't that just a matter of
> adding a new hook type and a new call site?

Sure. Its probably not easy to choose the correct callsites though, they
need to be somewhat different for create & alter in comparison to drop.

create & alter
* after the object is created/altered
drop:
* after the object is locked, but *before* its dropped

As far as I understood thats basically the behind the ddl_trace event
trigger mentioned earlier.

> > Also, even though I don't think its the right way *for this*, I think
> > pg_dump and pgadmin pretty much prove that it's possible?  The latter
> > even is out-of-core and has existed for multiple years.
> 
> Fair point.

Imo its already a good justification for a libpgdump, not that I am
volunteering, but ...

> > Its also not really fair to compare out-of-tree effort of maintaining
> > such a library to in-core support. pg_dump *needs* to be maintained, so
> > the additional maintenance overhead once the initial development is done
> > shouldn't really be higher than now. Lower if anything, due to getting
> > rid of a good bit of ugly code ;). There's no such effect out of core.
> 
> This I don't follow.  Nothing we might add to core to reverse-parse
> either the catalogs or the parse tree is going to make pg_dump go
> away.

I was still talking about the hypothetical libpgdump we don't really
need for this ;)

> > If youre also considering parsetree->SQL transformation under the
> > libpgdump umbrella its even less fair. The backend already has a *lot*
> > of infrastructure to regenerate sql from trees, even if its mostly
> > centered arround around DQL. A noticeable amount of that code is
> > unavailable to extensions (i.e. many are static funcs).
> > Imo its completely unreasonable to expose that code to the outside and
> > expect it to have a stable interface. We *will* need to rewrite parts of
> > that regularly.
> > For those reasons I think the only reasonable way to create textual DDL
> > is in the backend trying to allow outside code to do that will impose
> > far greater limitations.
> 
> I'm having trouble following this.  Can you restate?  I wasn't sure
> what you meant by libpqdump.  I assumed you were speaking of a
> parsetree->DDL or catalog->DDL facility.

Yea, that wasn't really clear, sorry for that.

What I was trying to say that I think doing parstree->text conversion
out of tree is noticeably more complex and essentially places a higher
maintenance burden on the project because
1) the core already has a lot of infrastructure for such conversions
2) any external project would either need to copy that infrastructure or
   make a large number of functions externally visible
3) any refactoring in that area would now break external tools even if
   it would be trivial to adjust potential in-core support for such a
   conversion

> > Some version of the event trigger patch contained partial support for
> > regenerating the DDL so it seems like a justified point there. Your
> > complained that suggestions about reusing object access hooks were
> > ignored, so mentioning that they currently don't provide *enough* (they
> > *do* provide a part, but it doesn't seem to be th

Re: [HACKERS] proposal: fix corner use case of variadic fuctions usage

[Tom Lane]

Stephen Frost  writes:
> [ quick review of patch ]

On reflection it seems to me that this is probably not a very good
approach overall.  Our general theory for functions taking ANY has
been that the core system just computes the arguments and leaves it
to the function to make sense of them.  Why should this be different
in the one specific case of VARIADIC ANY with a variadic array?

The approach is also inherently seriously inefficient.  Not only
does ExecMakeFunctionResult have to build a separate phony Const
node for each array element, but the variadic function itself can
have no idea that those Consts are all the same type, which means
it's going to do datatype lookup over again for each array element.
(format(), for instance, would have to look up the same type output
function over and over again.)  This might not be noticeable on toy
examples with a couple of array entries, but it'll be a large
performance problem on large arrays.

The patch also breaks any attempt that a variadic function might be
making to cache info about its argument types across calls.  I suppose
that the copying of the FmgrInfo is a hack to avoid crashes due to
called functions supposing that their flinfo->fn_extra data is still
valid for the new argument set.  Of course that's another pretty
significant performance hit, compounding the per-element hit.  Whereas
an ordinary variadic function that is given N arguments in a particular
query will probably do N datatype lookups and cache the info for the
life of the query, a variadic function called with this approach will
do one datatype lookup for each array element in each row of the query;
and there is no way to optimize that.

But large arrays have a worse problem: the approach flat out does
not work for arrays of more than FUNC_MAX_ARGS elements, because
there's no place to put their values in the FunctionCallInfo struct.
(As submitted, if the array is too big the patch will blithely stomp
all over memory with user-supplied data, making it not merely a crash
risk but probably an exploitable security hole.)

This last problem is, so far as I can see, unfixable within this
approach; surely we are not going to accept a feature that only works
for small arrays.  So I am going to mark the CF item rejected not just
RWF.

I believe that a workable approach would require having the function
itself act differently when the VARIADIC flag is set.  Currently there
is no way for ANY-taking functions to do this because we don't record
the use of the VARIADIC flag in FuncExpr, but it'd be reasonable to
change that, and probably then add a function similar to
get_fn_expr_rettype to dig it out of the FmgrInfo.  I don't know if
we could usefully provide any infrastructure beyond that for the case,
but it'd be worth thinking about whether there's any common behavior
for such functions.

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] logical changeset generation v4

[Robert Haas]

On Fri, Jan 18, 2013 at 11:33 AM, Alvaro Herrera
 wrote:
> Andres Freund wrote:
>
>> [09] Adjust all *Satisfies routines to take a HeapTuple instead of a 
>> HeapTupleHeader
>>
>> For timetravel access to the catalog we need to be able to lookup (cmin,
>> cmax) pairs of catalog rows when were 'inside' that TX. This patch just
>> adapts the signature of the *Satisfies routines to expect a HeapTuple
>> instead of a HeapTupleHeader. The amount of changes for that is fairly
>> low as the HeapTupleSatisfiesVisibility macro already expected the
>> former.
>>
>> It also makes sure the HeapTuple fields are setup in the few places that
>> didn't already do so.
>
> I had a look at this part.  Running the regression tests unveiled a case
> where the tableOid wasn't being set (and thus caused an assertion to
> fail), so I added that.  I also noticed that the additions to
> pruneheap.c are sometimes filling a tuple before it's strictly
> necessary, leading to wasted work.  Moved those too.
>
> Looks good to me as attached.

I took a quick look at this and am just curious why we're adding the
requirement that t_tableOid has to be initialized?

-- 
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] logical changeset generation v4

[Alvaro Herrera]

Alvaro Herrera wrote:

> I had a look at this part.  Running the regression tests unveiled a case
> where the tableOid wasn't being set (and thus caused an assertion to
> fail), so I added that.  I also noticed that the additions to
> pruneheap.c are sometimes filling a tuple before it's strictly
> necessary, leading to wasted work.  Moved those too.

Actually I missed that downthread there are some fixes to this part; I
had fixed one of these independently, but there's one I missed.  Added
that one too now (not attaching a new version).

(Also, it seems pointless to commit this unless we know for sure that
the downstream change that requires it is good; so I'm holding commit
until we've discussed the other stuff more thoroughly.  Per discussion
with Andres.)

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


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

Re: [HACKERS] Event Triggers: adding information

[Robert Haas]

On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund  wrote:
>> No, there's one also in heap_create_with_catalog.  Took me a minute to
>> find it, as it does not use InvokeObjectAccessHook.  The idea is that
>> OAT_POST_CREATE fires once per object creation, regardless of the
>> object type - table, column, whatever.
>
> Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
> comment explaining why we InvokeObjectAccessHook isn't used just for
> enhanced grepability.

I cannot parse that sentence, but I agree that the ungreppability of
it is suboptimal.  I resorted to git log -SInvokeObjectAccessHook -p
to find it.

>> I have been involved in PostgreSQL development for about 4.5 years
>> now.  This is less time than many people here, but it's still long
>> enough to say a whole lot of people ask for some variant of this idea,
>> and yet I have yet to see anybody produce a complete, working version
>> of this functionality and maintain it outside of the PostgreSQL tree
>> for one release cycle (did I miss something?).
>
> I don't really think thats a fair argument.
>
> For one, I didn't really ask for a libpgdump - I said that I don't see
> any way to generate re-executable SQL without it if we don't get the
> parse-tree but only the oid of the created object. Not to speak of the
> complexities of supporting ALTER that way (which is, as you note,
> basically not the way to do this).

Oh, OK.  I see.  Well, I've got no problem making the parse tree
available via InvokeObjectAccessHook.  Isn't that just a matter of
adding a new hook type and a new call site?

> Also, even though I don't think its the right way *for this*, I think
> pg_dump and pgadmin pretty much prove that it's possible?  The latter
> even is out-of-core and has existed for multiple years.

Fair point.

> Its also not really fair to compare out-of-tree effort of maintaining
> such a library to in-core support. pg_dump *needs* to be maintained, so
> the additional maintenance overhead once the initial development is done
> shouldn't really be higher than now. Lower if anything, due to getting
> rid of a good bit of ugly code ;). There's no such effect out of core.

This I don't follow.  Nothing we might add to core to reverse-parse
either the catalogs or the parse tree is going to make pg_dump go
away.

> If youre also considering parsetree->SQL transformation under the
> libpgdump umbrella its even less fair. The backend already has a *lot*
> of infrastructure to regenerate sql from trees, even if its mostly
> centered arround around DQL. A noticeable amount of that code is
> unavailable to extensions (i.e. many are static funcs).
> Imo its completely unreasonable to expose that code to the outside and
> expect it to have a stable interface. We *will* need to rewrite parts of
> that regularly.
> For those reasons I think the only reasonable way to create textual DDL
> is in the backend trying to allow outside code to do that will impose
> far greater limitations.

I'm having trouble following this.  Can you restate?  I wasn't sure
what you meant by libpqdump.  I assumed you were speaking of a
parsetree->DDL or catalog->DDL facility.

> Well, it is one of the major use-cases (or even *the* major use case)
> for event triggers that I heard of. So it seems a valid point in a
> dicussion on how to design the whole mechanism, doesn't it?

Yes, but is a separate problem from how we give control to the event
trigger (or object access hook).

> Some version of the event trigger patch contained partial support for
> regenerating the DDL so it seems like a justified point there. Your
> complained that suggestions about reusing object access hooks were
> ignored, so mentioning that they currently don't provide *enough* (they
> *do* provide a part, but it doesn't seem to be the most critical one)
> also seems justifyable.

Sure, but we could if we wanted decide to commit the partial support
for regenerating the DDL and tell people to use it via object access
hooks.  Of course, that would thin out even further the number of
people who would actually be using that code, which I fear will
already be too small to achieve bug-free operation in a reasonable
time.  If we add some hooks now and someone maintains the DDL
reverse-parsing code as an out-of-core replication solution for a few
releases, and that doesn't turn out to be hideous, I'd be a lot more
sanguine about incorporating it at that point.  I basically don't
think that there's any way we're going to commit something along those
lines now and not have it turn out to be a far bigger maintenance
headache than anyone wants.  What that tends to end up meaning in
practice is that Tom gets suckered into fixing it because nobody else
can take the time, and that's neither fair nor good for the project.

> NP, its good to keep the technical stuff here anyway... Stupid
> technology. In which business are we in again?

I don't know, maybe we can figure it out based on the objects in our
immediate v

Re: [HACKERS] logical changeset generation v4

[Alvaro Herrera]

Andres Freund wrote:

> [09] Adjust all *Satisfies routines to take a HeapTuple instead of a 
> HeapTupleHeader
> 
> For timetravel access to the catalog we need to be able to lookup (cmin,
> cmax) pairs of catalog rows when were 'inside' that TX. This patch just
> adapts the signature of the *Satisfies routines to expect a HeapTuple
> instead of a HeapTupleHeader. The amount of changes for that is fairly
> low as the HeapTupleSatisfiesVisibility macro already expected the
> former.
> 
> It also makes sure the HeapTuple fields are setup in the few places that
> didn't already do so.

I had a look at this part.  Running the regression tests unveiled a case
where the tableOid wasn't being set (and thus caused an assertion to
fail), so I added that.  I also noticed that the additions to
pruneheap.c are sometimes filling a tuple before it's strictly
necessary, leading to wasted work.  Moved those too.

Looks good to me as attached.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/contrib/pgrowlocks/pgrowlocks.c
--- b/contrib/pgrowlocks/pgrowlocks.c
***
*** 120,126  pgrowlocks(PG_FUNCTION_ARGS)
  		/* must hold a buffer lock to call HeapTupleSatisfiesUpdate */
  		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
  
! 		if (HeapTupleSatisfiesUpdate(tuple->t_data,
  	 GetCurrentCommandId(false),
  	 scan->rs_cbuf) == HeapTupleBeingUpdated)
  		{
--- 120,126 
  		/* must hold a buffer lock to call HeapTupleSatisfiesUpdate */
  		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
  
! 		if (HeapTupleSatisfiesUpdate(tuple,
  	 GetCurrentCommandId(false),
  	 scan->rs_cbuf) == HeapTupleBeingUpdated)
  		{
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***
*** 291,296  heapgetpage(HeapScanDesc scan, BlockNumber page)
--- 291,297 
  
  			loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
  			loctup.t_len = ItemIdGetLength(lpp);
+ 			loctup.t_tableOid = RelationGetRelid(scan->rs_rd);
  			ItemPointerSet(&(loctup.t_self), page, lineoff);
  
  			if (all_visible)
***
*** 1603,1609  heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
  
  		heapTuple->t_data = (HeapTupleHeader) PageGetItem(dp, lp);
  		heapTuple->t_len = ItemIdGetLength(lp);
! 		heapTuple->t_tableOid = relation->rd_id;
  		heapTuple->t_self = *tid;
  
  		/*
--- 1604,1610 
  
  		heapTuple->t_data = (HeapTupleHeader) PageGetItem(dp, lp);
  		heapTuple->t_len = ItemIdGetLength(lp);
! 		heapTuple->t_tableOid = RelationGetRelid(relation);
  		heapTuple->t_self = *tid;
  
  		/*
***
*** 1651,1657  heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
  		 * transactions.
  		 */
  		if (all_dead && *all_dead &&
! 			!HeapTupleIsSurelyDead(heapTuple->t_data, RecentGlobalXmin))
  			*all_dead = false;
  
  		/*
--- 1652,1658 
  		 * transactions.
  		 */
  		if (all_dead && *all_dead &&
! 			!HeapTupleIsSurelyDead(heapTuple, RecentGlobalXmin))
  			*all_dead = false;
  
  		/*
***
*** 1781,1786  heap_get_latest_tid(Relation relation,
--- 1782,1788 
  		tp.t_self = ctid;
  		tp.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  		tp.t_len = ItemIdGetLength(lp);
+ 		tp.t_tableOid = RelationGetRelid(relation);
  
  		/*
  		 * After following a t_ctid link, we might arrive at an unrelated
***
*** 2447,2458  heap_delete(Relation relation, ItemPointer tid,
  	lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
  	Assert(ItemIdIsNormal(lp));
  
  	tp.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  	tp.t_len = ItemIdGetLength(lp);
  	tp.t_self = *tid;
  
  l1:
! 	result = HeapTupleSatisfiesUpdate(tp.t_data, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{
--- 2449,2461 
  	lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
  	Assert(ItemIdIsNormal(lp));
  
+ 	tp.t_tableOid = RelationGetRelid(relation);
  	tp.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  	tp.t_len = ItemIdGetLength(lp);
  	tp.t_self = *tid;
  
  l1:
! 	result = HeapTupleSatisfiesUpdate(&tp, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{
***
*** 2817,2822  heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
--- 2820,2826 
  	lp = PageGetItemId(page, ItemPointerGetOffsetNumber(otid));
  	Assert(ItemIdIsNormal(lp));
  
+ 	oldtup.t_tableOid = RelationGetRelid(relation);
  	oldtup.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  	oldtup.t_len = ItemIdGetLength(lp);
  	oldtup.t_self = *otid;
***
*** 2829,2835  heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
  	 */
  
  l2:
! 	result = HeapTupleSatisfiesUpdate(oldtup.t_data, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{
--- 2833,2839 
  	 */
  
  l2:
! 	result = HeapTupleSatisfiesUpdate(&oldtup, cid, buffer);
  
  	if (re

Re: [HACKERS] HS locking broken in HEAD

[Andres Freund]

On 2013-01-18 17:26:00 +0100, Andres Freund wrote:
> On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
> > Andres Freund  writes:
> > > I am still stupefied nobody noticed that locking in HS (where just about
> > > all locks are going to be fast path locks) was completely broken for
> > > nearly two years.
> > 
> > IIUC it would only be broken for cases where activity was going on
> > concurrently in two different databases, which maybe isn't all that
> > common out in the field.  And for sure it isn't something we test much.
> 
> I think effectively it only was broken in Hot Standby. At least I don't
> immediately can think of a scenario where a strong lock is being acquired on a
> non-shared object in a different database.

Hrmpf, should have mentioned that the problem in HS is that the startup
process is doing exactly that, which is why it is broke there. It
acquires the exclusive locks shipped via WAL so the following
non-concurrency safe actions can be applied. And obviously its not
connected to any database while doing it...

I would have guessed its not that infrequent to run an ALTER TABLE or
somesuch while the standby is still running some longrunning query...

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] HS locking broken in HEAD

[Andres Freund]

On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > I am still stupefied nobody noticed that locking in HS (where just about
> > all locks are going to be fast path locks) was completely broken for
> > nearly two years.
> 
> IIUC it would only be broken for cases where activity was going on
> concurrently in two different databases, which maybe isn't all that
> common out in the field.  And for sure it isn't something we test much.

I think effectively it only was broken in Hot Standby. At least I don't
immediately can think of a scenario where a strong lock is being acquired on a
non-shared object in a different database.

> I wonder if it'd be practical to, say, run all the contrib regression
> tests concurrently in different databases of one installation.

I think it would be a good idea, but I don't immediately have an idea
how to implement it. It seems to me we would need to put the logic for
it into pg_regress? Otherwise the lifetime management of the shared
postmaster seems to get complicated.

What I would really like is to have some basic replication test scenario
in the regression tests. That seems like a dramatically undertested, but
pretty damn essential, part of the code.

The first handwavy guess I have of doing it is something like connecting
a second postmaster to the primary one at the start of the main
regression tests (requires changing the wal level again, yuck) and
fuzzyly comparing a pg_dump of the database remnants in both clusters at
the end of the regression tests.

Better ideas?

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] HS locking broken in HEAD

[Tom Lane]

Andres Freund  writes:
> I am still stupefied nobody noticed that locking in HS (where just about
> all locks are going to be fast path locks) was completely broken for
> nearly two years.

IIUC it would only be broken for cases where activity was going on
concurrently in two different databases, which maybe isn't all that
common out in the field.  And for sure it isn't something we test much.

I wonder if it'd be practical to, say, run all the contrib regression
tests concurrently in different databases of one installation.

regards, tom lane


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

[HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Andres Freund]

On 2013-01-18 11:11:50 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2013-01-18 10:33:16 -0500, Tom Lane wrote:
> >> Really I'd prefer not to move the backend definitions out of postgres.h
> >> at all, just because doing so will lose fifteen years of git history
> >> about those particular lines (or at least make it a lot harder to
> >> locate with git blame).
> 
> > The alternative seems to be sprinkling a few more #ifdef FRONTEND's in
> > postgres.h, if thats preferred I can prepare a patch for that although I
> > prefer my proposal.
> 
> Yeah, surrounding the existing definitions with #ifndef FRONTEND was
> what I was imagining.  But on reflection that seems pretty darn ugly,
> especially if the corresponding FRONTEND definitions are far away.
> Maybe we should just live with the git history disconnect.

FWIW, git blame -M -C, while noticeably more expensive, seems to be able to
connect the history:

d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  746) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  747) #define Assert(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  748)  Trap(!(condition), "FailedAssertion")
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  749) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  750) #define AssertMacro(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  751)  ((void) TrapMacro(!(condition), 
"FailedAssertion"))
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  752) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  753) #define AssertArg(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  754)  Trap(!(condition), "BadArgument")
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  755) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  756) #define AssertState(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  757)  Trap(!(condition), "BadState")
8118de2b src/include/c.h   (Andres Freund  2012-12-18 
00:58:36 +0100  758) 


Andres

-- 
 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 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Tom Lane]

Andres Freund  writes:
> On 2013-01-18 10:33:16 -0500, Tom Lane wrote:
>> Really I'd prefer not to move the backend definitions out of postgres.h
>> at all, just because doing so will lose fifteen years of git history
>> about those particular lines (or at least make it a lot harder to
>> locate with git blame).

> The alternative seems to be sprinkling a few more #ifdef FRONTEND's in
> postgres.h, if thats preferred I can prepare a patch for that although I
> prefer my proposal.

Yeah, surrounding the existing definitions with #ifndef FRONTEND was
what I was imagining.  But on reflection that seems pretty darn ugly,
especially if the corresponding FRONTEND definitions are far away.
Maybe we should just live with the git history disconnect.

Right at the moment the c.h location seems like the best bet.  I don't
see much advantage to Alvaro's proposal of a new header file.

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] HS locking broken in HEAD

[Andres Freund]

On 2013-01-18 10:15:20 -0500, Robert Haas wrote:
> On Thu, Jan 17, 2013 at 9:00 PM, Andres Freund  wrote:
> >> > I think it might also be a dangerous assumption for shared objects?
> >>
> >> Locks on shared objects can't be taken via the fast path.  In order to
> >> take a fast-path lock, a backend must be bound to a database and the
> >> locktag must be for a relation in that database.
> >
> > I assumed it wasn't allowed, but didn't find where that happens at first
> > - I found it now though (c.f. SetLocktagRelationOid).
> >
> >> > A patch minimally addressing the is attached, but it only addresses part
> >> > of the problem.
> >>
> >> Isn't the right fix to compare proc->databaseId to
> >> locktag->locktag_field1 rather than to MyDatabaseId?  The subsequent
> >> loop assumes that if the relid matches, the lock tag is an exact
> >> match, which will be correct with that rule but not the one you
> >> propose.
> >
> > I don't know much about the locking code, you're probably right. I also
> > didn't look very far after finding the guilty commit.
> >
> > (reading code)
> >
> > Yes, I think you're right, that seems to be the actually correct fix.
> >
> > I am a bit worried that there are more such assumptions in the code, but
> > I didn't find any, but I really don't know that code.
> 
> I found two instances of this.  See attached patch.

Yea, those are the two sites I had "fixed" (incorrectly) as well. I
looked a bit more yesterday night and I didn't find any additional
locations, so I hope we got this.

Yep, seems to work.

I am still stupefied nobody noticed that locking in HS (where just about
all locks are going to be fast path locks) was completely broken for
nearly two years.

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 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Andres Freund]

On 2013-01-18 10:33:16 -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Here's a different idea: move all the Assert() and StaticAssert() etc
> > definitions from c.h and postgres.h into a new header, say pgassert.h.
> > That header is included directly by postgres.h (just like palloc.h and
> > elog.h already are) so we don't have to touch the backend code at all.
> > Frontend programs that want that functionality can just #include
> > "pgassert.h" by themselves.  The definitions are (obviously) protected
> > by #ifdef FRONTEND so that it all works in both environments cleanly.
> 
> That might work.  Files using c.h would have to include this too, but
> as described it should work in either environment for them.  The other
> corner case is pg_controldata.c and other frontend programs that include
> postgres.h --- but it looks like they #define FRONTEND first, so they'd
> get the correct set of Assert definitions.
> 
> Whether it's really any better than just sticking them in c.h isn't
> clear.

I don't really see an advantage. To me providing sensible asserts sounds
like a good fit for c.h... And we already have StaticAssert*,
AssertVariableIsOfType* in c.h...

> Really I'd prefer not to move the backend definitions out of postgres.h
> at all, just because doing so will lose fifteen years of git history
> about those particular lines (or at least make it a lot harder to
> locate with git blame).

I can imagine some ugly macro solutions to that problem (pgassert.h
includes postgres.h with special defines), but that doesn't seem to be
warranted to me.

The alternative seems to be sprinkling a few more #ifdef FRONTEND's in
postgres.h, if thats preferred I can prepare a patch for that although I
prefer my proposal.

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] WIP patch for hint bit i/o mitigation

[Robert Haas]

On Fri, Jan 18, 2013 at 10:36 AM, Merlin Moncure  wrote:
> Any scenario that involves non-trivial amount of investigation or
> development should result in us pulling the patch for rework and
> resubmission in later 'festit's closing time as they say :-).

Amen.

-- 
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] Event Triggers: adding information

[Andres Freund]

On 2013-01-18 09:58:53 -0500, Robert Haas wrote:
> On Fri, Jan 18, 2013 at 9:07 AM, Andres Freund  wrote:
> > I don't have a problem reusing the object access infrastructure at all. I 
> > just
> > don't think its providing even remotely enough. You have (co-)written that
> > stuff, so you probably know more than I do, but could you explain to me how 
> > it
> > could be reused to replicate a CREATE TABLE?
> >
> > Problems I see:
> > - afaics for CREATE TABLE the only hook is in ATExecAddColumn
>
> No, there's one also in heap_create_with_catalog.  Took me a minute to
> find it, as it does not use InvokeObjectAccessHook.  The idea is that
> OAT_POST_CREATE fires once per object creation, regardless of the
> object type - table, column, whatever.

Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
comment explaining why we InvokeObjectAccessHook isn't used just for
enhanced grepability.

> > - No way to regenerate the table definition for execution on the remote 
> > system
> >   without creating libpqdump.
>
> IMHO, that is one of the really ugly problems that we haven't come up
> with a good solution for yet.  If you want to replicate DDL, you have
> basically three choices:
>
> 1. copy over the statement text that was used on the origin server and
> hope none of the corner cases bite you
> 2. come up with some way of reconstituting a DDL statement based on
> (a) the parse tree or (b) what the server actually decided to do
> 3. reconstitute the state of the object from the catalogs after the
> command has run
>
> (2a) differs from (2b) for things like CREATE INDEX, where the index
> name might be left for the server to determine, but when replicating
> you'd like to get the same name out.  (3) is workable for CREATE but
> not ALTER or DROP.
>
> The basic problem here is that (1) and (3) are not very
> reliable/complete and (2) is a lot of work and introduces a huge code
> maintenance burden.

I agree with that analysis.

> But it's unfair to pin that on the object-access
> hook mechanism

I agree. I don't think anybody has pinned it on it though.

> - any reverse-parsing or catalog-deconstruction
> solution for DDL is going to have that problem.  The decision we have
> to make as a community is whether we're prepared to support and
> maintain that code for the indefinite future.  Although I think it's
> easy to say "yes, because DDL replication would be really cool" - and
> I sure agree with that - I think it needs to be thought through a bit
> more deeply than that.
>
> I have been involved in PostgreSQL development for about 4.5 years
> now.  This is less time than many people here, but it's still long
> enough to say a whole lot of people ask for some variant of this idea,
> and yet I have yet to see anybody produce a complete, working version
> of this functionality and maintain it outside of the PostgreSQL tree
> for one release cycle (did I miss something?).

I don't really think thats a fair argument.

For one, I didn't really ask for a libpgdump - I said that I don't see
any way to generate re-executable SQL without it if we don't get the
parse-tree but only the oid of the created object. Not to speak of the
complexities of supporting ALTER that way (which is, as you note,
basically not the way to do this).

Also, even though I don't think its the right way *for this*, I think
pg_dump and pgadmin pretty much prove that it's possible?  The latter
even is out-of-core and has existed for multiple years.

Its also not really fair to compare out-of-tree effort of maintaining
such a library to in-core support. pg_dump *needs* to be maintained, so
the additional maintenance overhead once the initial development is done
shouldn't really be higher than now. Lower if anything, due to getting
rid of a good bit of ugly code ;). There's no such effect out of core.

If youre also considering parsetree->SQL transformation under the
libpgdump umbrella its even less fair. The backend already has a *lot*
of infrastructure to regenerate sql from trees, even if its mostly
centered arround around DQL. A noticeable amount of that code is
unavailable to extensions (i.e. many are static funcs).
Imo its completely unreasonable to expose that code to the outside and
expect it to have a stable interface. We *will* need to rewrite parts of
that regularly.
For those reasons I think the only reasonable way to create textual DDL
is in the backend trying to allow outside code to do that will impose
far greater limitations.

Whats the way you suggest to go on about this?

> There's a totally legitimate debate to be had here, but my feeling
> about what you're calling libpqdump is:

> - It's completely separate from event triggers.
> - It's completely separate from object access hooks.

Well, it is one of the major use-cases (or even *the* major use case)
for event triggers that I heard of. So it seems a valid point in a
dicussion on how to design the whole mechanism, doesn't it?

Some version of the event trig

Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Tom Lane]

Alvaro Herrera  writes:
> One slight problem with this is the common port/*.c idiom would require
> an extra include:

> #ifndef FRONTEND
> #include "postgres.h"
> #else
> #include "postgres_fe.h"
> #include "pgassert.h" /* <--- new line required here */
> #endif /* FRONTEND */

> If this is seen as a serious issue (I don't think so) then we would have
> to include pgassert.h into postgres_fe.h which would make the whole
> thing pointless (i.e. the same as just having the definitions in c.h).

We'd only need to add that when/if the file started to use asserts,
so I don't think it's a problem.  But still not sure it's better than
just putting the stuff in c.h.

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] WIP patch for hint bit i/o mitigation

[Merlin Moncure]

On Fri, Jan 18, 2013 at 8:57 AM, Atri Sharma  wrote:
>
> Hello all,
>
> Sorry for the delay in updating the hackers list with the current status.
>
> I recently did some profiling using perf on PostgreSQL 9.2 with and without 
> our patch.
>
> I noticed that maximum time is being spent on heapgettup function. Further 
> investigation and a bit of a hunch leads me to believe that we may be 
> adversely affecting the visibility map optimisation that directly interact 
> with the visibility functions, that our patch straight away affects.
>
> If this is the case, we may really need to get down to the design of our 
> patch, and maybe see which visibility function/functions we are affecting, 
> and see if we can mitigate the affect.
>
> Please let me know your inputs on this.

Any scenario that involves non-trivial amount of investigation or
development should result in us pulling the patch for rework and
resubmission in later 'festit's closing time as they say :-).

merlin


-- 
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 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Alvaro Herrera]

Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
> > > Here's a trivially updated patch which also defines AssertArg() for
> > > FRONTEND-ish environments since Alvaro added one in xlogreader.c.
> > 
> > This time for real. Please.
> 
> Here's a different idea: move all the Assert() and StaticAssert() etc
> definitions from c.h and postgres.h into a new header, say pgassert.h.
> That header is included directly by postgres.h (just like palloc.h and
> elog.h already are) so we don't have to touch the backend code at all.
> Frontend programs that want that functionality can just #include
> "pgassert.h" by themselves.  The definitions are (obviously) protected
> by #ifdef FRONTEND so that it all works in both environments cleanly.

One slight problem with this is the common port/*.c idiom would require
an extra include:

#ifndef FRONTEND
#include "postgres.h"
#else
#include "postgres_fe.h"
#include "pgassert.h"   /* <--- new line required here */
#endif /* FRONTEND */

If this is seen as a serious issue (I don't think so) then we would have
to include pgassert.h into postgres_fe.h which would make the whole
thing pointless (i.e. the same as just having the definitions in c.h).

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


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

Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Tom Lane]

Alvaro Herrera  writes:
> Here's a different idea: move all the Assert() and StaticAssert() etc
> definitions from c.h and postgres.h into a new header, say pgassert.h.
> That header is included directly by postgres.h (just like palloc.h and
> elog.h already are) so we don't have to touch the backend code at all.
> Frontend programs that want that functionality can just #include
> "pgassert.h" by themselves.  The definitions are (obviously) protected
> by #ifdef FRONTEND so that it all works in both environments cleanly.

That might work.  Files using c.h would have to include this too, but
as described it should work in either environment for them.  The other
corner case is pg_controldata.c and other frontend programs that include
postgres.h --- but it looks like they #define FRONTEND first, so they'd
get the correct set of Assert definitions.

Whether it's really any better than just sticking them in c.h isn't
clear.

Really I'd prefer not to move the backend definitions out of postgres.h
at all, just because doing so will lose fifteen years of git history
about those particular lines (or at least make it a lot harder to
locate with git blame).

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: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Alvaro Herrera]

Andres Freund wrote:
> On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
> > Here's a trivially updated patch which also defines AssertArg() for
> > FRONTEND-ish environments since Alvaro added one in xlogreader.c.
> 
> This time for real. Please.

Here's a different idea: move all the Assert() and StaticAssert() etc
definitions from c.h and postgres.h into a new header, say pgassert.h.
That header is included directly by postgres.h (just like palloc.h and
elog.h already are) so we don't have to touch the backend code at all.
Frontend programs that want that functionality can just #include
"pgassert.h" by themselves.  The definitions are (obviously) protected
by #ifdef FRONTEND so that it all works in both environments cleanly.

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


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

Re: [HACKERS] HS locking broken in HEAD

[Robert Haas]

On Thu, Jan 17, 2013 at 9:00 PM, Andres Freund  wrote:
>> > I think it might also be a dangerous assumption for shared objects?
>>
>> Locks on shared objects can't be taken via the fast path.  In order to
>> take a fast-path lock, a backend must be bound to a database and the
>> locktag must be for a relation in that database.
>
> I assumed it wasn't allowed, but didn't find where that happens at first
> - I found it now though (c.f. SetLocktagRelationOid).
>
>> > A patch minimally addressing the is attached, but it only addresses part
>> > of the problem.
>>
>> Isn't the right fix to compare proc->databaseId to
>> locktag->locktag_field1 rather than to MyDatabaseId?  The subsequent
>> loop assumes that if the relid matches, the lock tag is an exact
>> match, which will be correct with that rule but not the one you
>> propose.
>
> I don't know much about the locking code, you're probably right. I also
> didn't look very far after finding the guilty commit.
>
> (reading code)
>
> Yes, I think you're right, that seems to be the actually correct fix.
>
> I am a bit worried that there are more such assumptions in the code, but
> I didn't find any, but I really don't know that code.

I found two instances of this.  See attached patch.

Can you test whether this fixes it for you?

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


fix-mydatabaseid-compare.patch
Description: Binary 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] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Andres Freund]

On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
> Here's a trivially updated patch which also defines AssertArg() for
> FRONTEND-ish environments since Alvaro added one in xlogreader.c.

This time for real. Please.

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
>From 0190dd4d9a6d8e638727eaee71cb1390e6bbe88e Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 8 Jan 2013 17:59:10 +0100
Subject: [PATCH] Centralize Assert* macros into c.h so its common between
 backend/frontend

c.h already had parts of the assert support (StaticAssert*) and its the shared
file between postgres.h and postgres_fe.h. This makes it easier to build
frontend programs which have to do the hack.
---
 src/include/c.h   | 67 +++
 src/include/postgres.h| 54 ++
 src/include/postgres_fe.h | 12 -
 3 files changed, 69 insertions(+), 64 deletions(-)

diff --git a/src/include/c.h b/src/include/c.h
index 59af5b5..e2aefa9 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -694,6 +694,73 @@ typedef NameData *Name;
 
 
 /*
+ * USE_ASSERT_CHECKING, if defined, turns on all the assertions.
+ * - plai  9/5/90
+ *
+ * It should _NOT_ be defined in releases or in benchmark copies
+ */
+
+/*
+ * Assert() can be used in both frontend and backend code. In frontend code it
+ * just calls the standard assert, if it's available. If use of assertions is
+ * not configured, it does nothing.
+ */
+#ifndef USE_ASSERT_CHECKING
+
+#define Assert(condition)
+#define AssertMacro(condition)	((void)true)
+#define AssertArg(condition)
+#define AssertState(condition)
+
+#elif defined FRONTEND
+
+#include 
+#define Assert(p) assert(p)
+#define AssertMacro(p)	((void) assert(p))
+#define AssertArg(condition) assert(condition)
+#define AssertState(condition) assert(condition)
+
+#else /* USE_ASSERT_CHECKING && FRONTEND */
+
+/*
+ * Trap
+ *		Generates an exception if the given condition is true.
+ */
+#define Trap(condition, errorType) \
+	do { \
+		if ((assert_enabled) && (condition)) \
+			ExceptionalCondition(CppAsString(condition), (errorType), \
+ __FILE__, __LINE__); \
+	} while (0)
+
+/*
+ *	TrapMacro is the same as Trap but it's intended for use in macros:
+ *
+ *		#define foo(x) (AssertMacro(x != 0), bar(x))
+ *
+ *	Isn't CPP fun?
+ */
+#define TrapMacro(condition, errorType) \
+	((bool) ((! assert_enabled) || ! (condition) || \
+			 (ExceptionalCondition(CppAsString(condition), (errorType), \
+   __FILE__, __LINE__), 0)))
+
+#define Assert(condition) \
+		Trap(!(condition), "FailedAssertion")
+
+#define AssertMacro(condition) \
+		((void) TrapMacro(!(condition), "FailedAssertion"))
+
+#define AssertArg(condition) \
+		Trap(!(condition), "BadArgument")
+
+#define AssertState(condition) \
+		Trap(!(condition), "BadState")
+
+#endif /* USE_ASSERT_CHECKING && !FRONTEND */
+
+
+/*
  * Macros to support compile-time assertion checks.
  *
  * If the "condition" (a compile-time-constant expression) evaluates to false,
diff --git a/src/include/postgres.h b/src/include/postgres.h
index b6e922f..bbe125a 100644
--- a/src/include/postgres.h
+++ b/src/include/postgres.h
@@ -25,7 +25,7 @@
  *	  ---	
  *		1)		variable-length datatypes (TOAST support)
  *		2)		datum type + support macros
- *		3)		exception handling definitions
+ *		3)		exception handling
  *
  *	 NOTES
  *
@@ -627,62 +627,12 @@ extern Datum Float8GetDatum(float8 X);
 
 
 /* 
- *Section 3:	exception handling definitions
- *			Assert, Trap, etc macros
+ *Section 3:	exception handling backend support
  * 
  */
 
 extern PGDLLIMPORT bool assert_enabled;
 
-/*
- * USE_ASSERT_CHECKING, if defined, turns on all the assertions.
- * - plai  9/5/90
- *
- * It should _NOT_ be defined in releases or in benchmark copies
- */
-
-/*
- * Trap
- *		Generates an exception if the given condition is true.
- */
-#define Trap(condition, errorType) \
-	do { \
-		if ((assert_enabled) && (condition)) \
-			ExceptionalCondition(CppAsString(condition), (errorType), \
- __FILE__, __LINE__); \
-	} while (0)
-
-/*
- *	TrapMacro is the same as Trap but it's intended for use in macros:
- *
- *		#define foo(x) (AssertMacro(x != 0), bar(x))
- *
- *	Isn't CPP fun?
- */
-#define TrapMacro(condition, errorType) \
-	((bool) ((! assert_enabled) || ! (condition) || \
-			 (ExceptionalCondition(CppAsString(condition), (errorType), \
-   __FILE__, __LINE__), 0)))
-
-#ifndef USE_ASSERT_CHECKING
-#define Assert(condition)
-#define AssertMacro(condition)	((void)true)
-#define AssertArg(condition)
-#define AssertState(condition)
-#else
-#define Assert(condition) \
-		Trap(!(condition), "FailedAssertion")
-
-#define AssertMacro(condition) \
-		((void) 

[HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

[Andres Freund]

On 2013-01-09 15:07:10 -0500, Tom Lane wrote:
> I would like to know if other people get comparable results on other
> hardware (non-Intel hardware would be especially interesting).  If this
> result holds up across a range of platforms, I'll withdraw my objection
> to making palloc a plain function.

Unfortunately nobody spoke up and I don't have any non-intel hardware
anymore. Well, aside from my phone that is...

Updated patch attached, the previous version missed some contrib modules.

I like the diffstat:
 41 files changed, 224 insertions(+), 636 deletions(-)

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
>From e4960b1659c8bc33661ff07b2ba3cccef653c8fc Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 9 Jan 2013 12:05:37 +0100
Subject: [PATCH] Provide a common malloc wrappers and palloc et al. emulation
 for frontend'ish environs

---
 contrib/oid2name/oid2name.c|  52 +
 contrib/pg_upgrade/pg_upgrade.h|   5 +-
 contrib/pg_upgrade/util.c  |  49 -
 contrib/pgbench/pgbench.c  |  54 +-
 src/backend/storage/file/copydir.c |  12 +--
 src/backend/utils/mmgr/mcxt.c  |  78 +++-
 src/bin/initdb/initdb.c|  40 +-
 src/bin/pg_basebackup/pg_basebackup.c  |   2 +-
 src/bin/pg_basebackup/pg_receivexlog.c |   1 +
 src/bin/pg_basebackup/receivelog.c |   1 +
 src/bin/pg_basebackup/streamutil.c |  38 +-
 src/bin/pg_basebackup/streamutil.h |   4 -
 src/bin/pg_ctl/pg_ctl.c|  39 +-
 src/bin/pg_dump/Makefile   |   6 +-
 src/bin/pg_dump/common.c   |   1 -
 src/bin/pg_dump/compress_io.c  |   1 -
 src/bin/pg_dump/dumpmem.c  |  76 ---
 src/bin/pg_dump/dumpmem.h  |  22 --
 src/bin/pg_dump/dumputils.c|   1 -
 src/bin/pg_dump/dumputils.h|   1 +
 src/bin/pg_dump/pg_backup_archiver.c   |   1 -
 src/bin/pg_dump/pg_backup_custom.c |   2 +-
 src/bin/pg_dump/pg_backup_db.c |   1 -
 src/bin/pg_dump/pg_backup_directory.c  |   1 -
 src/bin/pg_dump/pg_backup_null.c   |   1 -
 src/bin/pg_dump/pg_backup_tar.c|   1 -
 src/bin/pg_dump/pg_dump.c  |   1 -
 src/bin/pg_dump/pg_dump_sort.c |   1 -
 src/bin/pg_dump/pg_dumpall.c   |   1 -
 src/bin/pg_dump/pg_restore.c   |   1 -
 src/bin/pg_resetxlog/pg_resetxlog.c|   5 +-
 src/bin/psql/common.c  |  50 -
 src/bin/psql/common.h  |  10 +--
 src/bin/scripts/common.c   |  49 -
 src/bin/scripts/common.h   |   5 +-
 src/include/port/palloc.h  |  19 +
 src/include/utils/palloc.h |  12 +--
 src/port/Makefile  |   8 +-
 src/port/dirmod.c  |  75 +--
 src/port/palloc.c  | 129 +
 src/tools/msvc/Mkvcbuild.pm|   4 +-
 41 files changed, 224 insertions(+), 636 deletions(-)
 delete mode 100644 src/bin/pg_dump/dumpmem.c
 delete mode 100644 src/bin/pg_dump/dumpmem.h
 create mode 100644 src/include/port/palloc.h
 create mode 100644 src/port/palloc.c

diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index a666731..dfd8105 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -9,6 +9,8 @@
  */
 #include "postgres_fe.h"
 
+#include "port/palloc.h"
+
 #include 
 #ifdef HAVE_GETOPT_H
 #include 
@@ -50,9 +52,6 @@ struct options
 /* function prototypes */
 static void help(const char *progname);
 void		get_opts(int, char **, struct options *);
-void	   *pg_malloc(size_t size);
-void	   *pg_realloc(void *ptr, size_t size);
-char	   *pg_strdup(const char *str);
 void		add_one_elt(char *eltname, eary *eary);
 char	   *get_comma_elts(eary *eary);
 PGconn	   *sql_conn(struct options *);
@@ -201,53 +200,6 @@ help(const char *progname)
 		   progname, progname);
 }
 
-void *
-pg_malloc(size_t size)
-{
-	void	   *ptr;
-
-	/* Avoid unportable behavior of malloc(0) */
-	if (size == 0)
-		size = 1;
-	ptr = malloc(size);
-	if (!ptr)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return ptr;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
-	void	   *result;
-
-	/* Avoid unportable behavior of realloc(NULL, 0) */
-	if (ptr == NULL && size == 0)
-		size = 1;
-	result = realloc(ptr, size);
-	if (!result)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return result;
-}
-
-char *
-pg_strdup(const char *str)
-{
-	char	   *result = strdup(str);
-
-	if (!result)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return result;
-}
-
 /*
  * add_one_elt
  *
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index d5c3fa9..a917b5b 100644
--- a/contrib/pg_upgrade/pg_upgrad

Re: [HACKERS] Event Triggers: adding information

[Simon Riggs]

On 18 January 2013 02:48, Tom Lane  wrote:
> Andres Freund  writes:
>> I have no problem requiring C code to use the even data, be it via hooks
>> or via C functions called from event triggers. The problem I have with
>> putting in some hooks is that I doubt that you can find sensible spots
>> with enough information to actually recreate the DDL for a remote system
>> without doing most of the work for command triggers.
>
> "most of the work"?  No, I don't think so.  Here's the thing that's
> bothering me about that: if the use-case that everybody is worried about
> is replication, then triggers of any sort are the Wrong Thing, IMO.
>
> The difference between a replication hook and a trigger is that a
> replication hook has no business changing any state of the local system,
> whereas a trigger *has to be expected to change the state of the local
> system* because if it has no side-effects you might as well not bother
> firing it.  And the fear of having to cope with arbitrary side-effects
> occuring in the midst of DDL is about 80% of my angst with this whole
> concept.
>
> If we're only interested in replication, let's put in some hooks whose
> contract does not allow for side-effects on the local catalogs, and be
> done.  Otherwise we'll be putting in man-years of unnecessary (or at
> least unnecessary for this use-case) work.


I would be happy to see something called "replication triggers" or
"replication hooks" committed.

The things I want to be able to do are:
* Replicate DDL for use on other systems, in multiple ways selected by
user, possibly >1 at same time
* Put in a block to prevent all DDL, for use during upgrades or just a
general lockdown, then remove it again when complete.
* Perform auditing of DDL statements (which requires more info than
simply the text submitted)

If there are questions about wider functionality, or risks with that,
then reducing the functionality is OK, for me. The last thing we need
is a security hole introduced by this, or weird behaviour from people
interfering with things. (If you want that, write an executor plugin
and cook your own spaghetti).

We can do this by declaring clearly that there are limits on what can
be achieved with event triggers, perhaps even testing to check bad
things haven't been allowed to occur. I don't see that as translating
into massive change in the patch, just focusing on the main cases,
documenting that and perhaps introducing some restrictions on wider
usage. We will likely slowly expand the functionality of event
triggers over time, so keeping things general still works as a long
range goal.

-- 
 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] recursive view syntax

[Stephen Frost]

* Peter Eisentraut (pete...@gmx.net) wrote:
> I noticed we don't implement the recursive view syntax, even though it's
> part of the standard SQL feature set for recursive queries.  Here is a
> patch to add that.  It basically converts
> 
> CREATE RECURSIVE VIEW name (columns) AS SELECT ...;
> 
> to
> 
> CREATE VIEW name AS WITH RECURSIVE name (columns) AS (SELECT ...) SELECT
> columns FROM name;

I've done another review of this patch and it looks pretty good to me.
My only complaint is that there isn't a single comment inside
makeRecursiveViewSelect().

One other thought is- I'm guessing this isn't going to work:

CREATE RECURSIVE VIEW name (columns) AS WITH ... SELECT ...;

Does the spec explicitly allow or disallow that?  Should we provide any
comments about it?

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Event Triggers: adding information

[Robert Haas]

On Fri, Jan 18, 2013 at 9:07 AM, Andres Freund  wrote:
> On 2013-01-17 22:39:18 -0500, Robert Haas wrote:
>> On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund  
>> wrote:
>> > I have no problem requiring C code to use the even data, be it via hooks
>> > or via C functions called from event triggers. The problem I have with
>> > putting in some hooks is that I doubt that you can find sensible spots
>> > with enough information to actually recreate the DDL for a remote system
>> > without doing most of the work for command triggers.
>>
>> It should be noted that the point of KaiGai's work over the last three
>> years has been to solve exactly this problem.  Well, KaiGai wants to
>> check security rather than do replication, but he wants to be able
>> grovel through the entire node tree and make security decisions based
>> on stuff core PG doesn't care about, so in effect the requirements are
>> identical.  Calling the facility "event triggers" rather than "object
>> access hooks" doesn't make the underlying problem any easier to solve.
>>  I actually believe that the object access hook stuff is getting
>> pretty close to a usable solution if you don't mind coding in C, but
>> I've had trouble convincing anyone else of that.
>>
>> I find it a shame that it hasn't been taken more seriously, because it
>> really does solve the same problem.  sepgsql, for example, has no
>> trouble at all checking permissions for dropped objects.  You can't
>> call procedural code from the spot where we've got that hook, but you
>> sure can call C code, with the usual contract that if it breaks you
>> get to keep both pieces.  The CREATE stuff works fine too.  Support
>> for ALTER is not all there yet, but that's because it's a hard
>> problem.
>
> I don't have a problem reusing the object access infrastructure at all. I just
> don't think its providing even remotely enough. You have (co-)written that
> stuff, so you probably know more than I do, but could you explain to me how it
> could be reused to replicate a CREATE TABLE?
>
> Problems I see:
> - afaics for CREATE TABLE the only hook is in ATExecAddColumn

No, there's one also in heap_create_with_catalog.  Took me a minute to
find it, as it does not use InvokeObjectAccessHook.  The idea is that
OAT_POST_CREATE fires once per object creation, regardless of the
object type - table, column, whatever.

> - no access to the CreateStm, making it impossible to decipher whether e.g. a
>   sequence was created as part of this or not

Yep, that's a problem.  We could of course add additional hook sites
with relevant context information - that's what this infrastructure is
supposed to allow for.

> - No way to regenerate the table definition for execution on the remote system
>   without creating libpqdump.

IMHO, that is one of the really ugly problems that we haven't come up
with a good solution for yet.  If you want to replicate DDL, you have
basically three choices:

1. copy over the statement text that was used on the origin server and
hope none of the corner cases bite you
2. come up with some way of reconstituting a DDL statement based on
(a) the parse tree or (b) what the server actually decided to do
3. reconstitute the state of the object from the catalogs after the
command has run

(2a) differs from (2b) for things like CREATE INDEX, where the index
name might be left for the server to determine, but when replicating
you'd like to get the same name out.  (3) is workable for CREATE but
not ALTER or DROP.

The basic problem here is that (1) and (3) are not very
reliable/complete and (2) is a lot of work and introduces a huge code
maintenance burden.  But it's unfair to pin that on the object-access
hook mechanism - any reverse-parsing or catalog-deconstruction
solution for DDL is going to have that problem.  The decision we have
to make as a community is whether we're prepared to support and
maintain that code for the indefinite future.  Although I think it's
easy to say "yes, because DDL replication would be really cool" - and
I sure agree with that - I think it needs to be thought through a bit
more deeply than that.

I have been involved in PostgreSQL development for about 4.5 years
now.  This is less time than many people here, but it's still long
enough to say a whole lot of people ask for some variant of this idea,
and yet I have yet to see anybody produce a complete, working version
of this functionality and maintain it outside of the PostgreSQL tree
for one release cycle (did I miss something?).  If we pull that
functionality into core, that's just a way of taking work that
nobody's been willing to do and force the responsibility to be spread
across the whole development group.  Now, sometimes it is OK to do
that, but sometimes it means that we're just bolting more things onto
the already-long list of reasons for which patches can be rejected.
Anybody who is now feeling uncomfortable at the prospect of not having
that facility in core ought to think about how they'll feel

Re: [HACKERS] WIP patch for hint bit i/o mitigation

[Atri Sharma]

On 18-Jan-2013, at 17:04, Craig Ringer  wrote:

> On 12/14/2012 09:57 PM, Amit Kapila wrote:
>>> 
>>> I need to validate the vacuum results. It's possible that this is
>>> solvable by tweaking xmin check inside vacuum. Assuming that's fixed,
>>> the question stands: do the results justify the change?  I'd argue
>>> 'maybe'
>> We can try with change (assuming change is small) and see if the performance
>> gain is good, then discuss whether it really justifies.
>> I think the main reason for Vacuum performance hit is that in the test pages
>> are getting dirty only due to setting of hint bit
>> by Vacuum. 
>> 
>>> -- I'd like to see the bulk insert performance hit reduced if
>>> possible.
>> I think if we can improve performance for bulk-insert case, then this patch
>> has much more value.
> Has there been any movement in this - more benchmarks and data showing
> that it really does improve performance, or that it really isn't helpful?
> 
> -- 
> Craig Ringer   http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

Hello all,

Sorry for the delay in updating the hackers list with the current status.

I recently did some profiling using perf on PostgreSQL 9.2 with and without our 
patch.

I noticed that maximum time is being spent on heapgettup function. Further 
investigation and a bit of a hunch leads me to believe that we may be adversely 
affecting the visibility map optimisation that directly interact with the 
visibility functions, that our patch straight away affects.

If this is the case, we may really need to get down to the design of our patch, 
and maybe see which visibility function/functions we are affecting, and see if 
we can mitigate the affect.

Please let me know your inputs on this.

Regards,

Atri


> 


-- 
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] proposal: fix corner use case of variadic fuctions usage

[Stephen Frost]

Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
> Now I fixed these issues and I hope  so it will work on all platforms

As mentioned on the commitfest application, this needs documentation.
That is not the responsibility of the committer; if you need help, then
please ask for it.

I've also done a quick review of it.

The massive if() block being added to execQual.c:ExecMakeFunctionResult
really looks like it might be better pulled out into a function of its
own.

What does "use element_type depends for dimensions" mean and why is it
a ToDo?  When will it be done?

Overall, the comments really need to be better.  Things like this:

+   /* create short lived copies of fmgr data */
+   fmgr_info_copy(&sfinfo, fcinfo->flinfo, fcinfo->flinfo->fn_mcxt);
+   memcpy(scinfo, fcinfo, sizeof(FunctionCallInfoData));
+   scinfo->flinfo = &sfinfo;


Really don't cut it, imv.  *Why* are we creating a copy of the fmgr
data?  Why does it need to be short lived?  And what is going to happen
later when you do this?:

fcinfo = scinfo;
MemoryContextSwitchTo(oldContext);

Is there any reason to worry about the fact that fcache->fcinfo_data no
longer matches the fcinfo that we're working on?  What if there are
other references made to it in the future?  These memcpy's and pointer
foolishness really don't strike me as being a well thought out approach.

There are other similar comments throughout which really need to be
rewritten to address why we're doing something, not what is being done-
you can read the code for that.

Marking this Waiting on Author.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

[Andres Freund]

On 2013-01-08 15:55:24 -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Andres Freund wrote:
> >> Sorry, misremembered the problem somewhat. The problem is that code that
> >> includes postgres.h atm ends up with ExceptionalCondition() et
> >> al. declared even if FRONTEND is defined. So if anything uses an assert
> >> you need to provide wrappers for those which seems nasty. If they are
> >> provided centrally and check for FRONTEND that problem doesn't exist.
> 
> > I think the right fix here is to fix things so that postgres.h is not
> > necessary.  How hard is that?  Maybe it just requires some more
> > reshuffling of xlog headers.
> 
> That would definitely be the ideal fix.  In general, #include'ing
> postgres.h into code that's not backend code opens all kinds of hazards
> that are likely to bite us sooner or later; the incompatibility of the
> Assert definitions is just the tip of that iceberg.  (In the past we've
> had issues around  providing different definitions in the two
> environments, for example.)

I agree that going in that direction is a good thing, but imo that doesn't
really has any bearing on whether this patch is a good/bad idea...

> But having said that, I'm also now remembering that we have files in
> src/port/ and probably elsewhere that try to work in both environments
> by just #include'ing c.h directly (relying on the Makefile to supply
> -DFRONTEND or not).  If we wanted to support Assert use in such files,
> we would have to move at least the Assert() macro definitions into c.h
> as Andres is proposing.  Now, I've always thought that #include'ing c.h
> directly was kind of an ugly hack, but I don't have a better design than
> that to offer ATM.

Imo having some common dumping ground for things that concern both environments
is a good idea. I don't think c.h would be the best place for it when
redesigning from ground up, but were not doing that so imo its acceptable as
long as we take care not to let it grow too much.

Here's a trivially updated patch which also defines AssertArg() for
FRONTEND-ish environments since Alvaro added one in xlogreader.c.

Do we agree this is the way forward, at least for now?

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: Slave enters in recovery and promotes when WAL stream with master is cut + delay master/slave

[Alvaro Herrera]

Andres Freund escribió:
> On 2013-01-18 08:24:31 +0900, Michael Paquier wrote:
> 
> > The replication delays are still here.
> 
> That one is caused by this nice bug, courtesy of yours truly:
> diff --git a/src/backend/access/transam/xlog.c 
> b/src/backend/access/transam/xlog.c
> index 90ba32e..1174493 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -8874,7 +8874,7 @@ retry:
> /* See if we need to retrieve more data */
> if (readFile < 0 ||
> (readSource == XLOG_FROM_STREAM &&
> -receivedUpto <= targetPagePtr + reqLen))
> +receivedUpto < targetPagePtr + reqLen))
> {
> if (StandbyMode)
> {

Pushed.

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


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

Re: [HACKERS] WIP patch for hint bit i/o mitigation

[Merlin Moncure]

On Fri, Jan 18, 2013 at 5:34 AM, Craig Ringer  wrote:
> On 12/14/2012 09:57 PM, Amit Kapila wrote:
>>>
>>> I need to validate the vacuum results. It's possible that this is
>>> solvable by tweaking xmin check inside vacuum. Assuming that's fixed,
>>> the question stands: do the results justify the change?  I'd argue
>>> 'maybe'
>> We can try with change (assuming change is small) and see if the performance
>> gain is good, then discuss whether it really justifies.
>> I think the main reason for Vacuum performance hit is that in the test pages
>> are getting dirty only due to setting of hint bit
>> by Vacuum.
>>
>>> -- I'd like to see the bulk insert performance hit reduced if
>>> possible.
>> I think if we can improve performance for bulk-insert case, then this patch
>> has much more value.
> Has there been any movement in this - more benchmarks and data showing
> that it really does improve performance, or that it really isn't helpful?

Atri is working on that.  I don't know if he's going to pull it out
though in time so we may have to pull the patch from this fest.  My
take on the current patch is that the upside case is pretty clear but
the bulk insert performance impact needs to be figured out and
mitigated (that's what Atri is working on).

merlin


-- 
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] Event Triggers: adding information

[Andres Freund]

On 2013-01-17 22:39:18 -0500, Robert Haas wrote:
> On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund  wrote:
> > I have no problem requiring C code to use the even data, be it via hooks
> > or via C functions called from event triggers. The problem I have with
> > putting in some hooks is that I doubt that you can find sensible spots
> > with enough information to actually recreate the DDL for a remote system
> > without doing most of the work for command triggers.
> 
> It should be noted that the point of KaiGai's work over the last three
> years has been to solve exactly this problem.  Well, KaiGai wants to
> check security rather than do replication, but he wants to be able
> grovel through the entire node tree and make security decisions based
> on stuff core PG doesn't care about, so in effect the requirements are
> identical.  Calling the facility "event triggers" rather than "object
> access hooks" doesn't make the underlying problem any easier to solve.
>  I actually believe that the object access hook stuff is getting
> pretty close to a usable solution if you don't mind coding in C, but
> I've had trouble convincing anyone else of that.
> 
> I find it a shame that it hasn't been taken more seriously, because it
> really does solve the same problem.  sepgsql, for example, has no
> trouble at all checking permissions for dropped objects.  You can't
> call procedural code from the spot where we've got that hook, but you
> sure can call C code, with the usual contract that if it breaks you
> get to keep both pieces.  The CREATE stuff works fine too.  Support
> for ALTER is not all there yet, but that's because it's a hard
> problem.

I don't have a problem reusing the object access infrastructure at all. I just
don't think its providing even remotely enough. You have (co-)written that
stuff, so you probably know more than I do, but could you explain to me how it
could be reused to replicate a CREATE TABLE?

Problems I see:
- afaics for CREATE TABLE the only hook is in ATExecAddColumn
- no access to the CreateStm, making it impossible to decipher whether e.g. a
  sequence was created as part of this or not
- No way to regenerate the table definition for execution on the remote system
  without creating libpqdump.

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] Materialized views WIP patch

[Thom Brown]

On 17 January 2013 16:03, Thom Brown  wrote:

> On 16 January 2013 17:25, Thom Brown  wrote:
>
>> On 16 January 2013 17:20, Kevin Grittner  wrote:
>>
>>> Thom Brown wrote:
>>>
>>> > Some weirdness:
>>> >
>>> > postgres=# CREATE VIEW v_test2 AS SELECT 1 moo;
>>> > CREATE VIEW
>>> > postgres=# CREATE MATERIALIZED VIEW mv_test2 AS SELECT moo, 2*moo FROM
>>> > v_test2 UNION ALL SELECT moo, 3*moo FROM v_test2;
>>> > SELECT 2
>>> > postgres=# \d+ mv_test2
>>> >  Materialized view "public.mv_test2"
>>> >  Column | Type | Modifiers | Storage | Stats target | Description
>>> > --+-+---+-+--+-
>>> >  moo | integer | | plain | |
>>> >  ?column? | integer | | plain | |
>>> > View definition:
>>> >  SELECT "*SELECT* 1".moo, "*SELECT* 1"."?column?";
>>>
>>> You are very good at coming up with these, Thom!
>>>
>>> Will investigate.
>>>
>>> Can you confirm that *selecting* from the MV works as you would
>>> expect; it is just the presentation in \d+ that's a problem?
>>>
>>
>> Yes, nothing wrong with using the MV, or refreshing it:
>>
>> postgres=# TABLE mv_test2;
>>  moo | ?column?
>> -+--
>>1 |2
>>1 |3
>> (2 rows)
>>
>> postgres=# SELECT * FROM mv_test2;
>>  moo | ?column?
>> -+--
>>1 |2
>>1 |3
>> (2 rows)
>>
>> postgres=# REFRESH MATERIALIZED VIEW mv_test2;
>> REFRESH MATERIALIZED VIEW
>>
>> But a pg_dump of the MV has the same issue as the view definition:
>>
>> --
>> -- Name: mv_test2; Type: MATERIALIZED VIEW; Schema: public; Owner: thom;
>> Tablespace:
>> --
>>
>> CREATE MATERIALIZED VIEW mv_test2 (
>> moo,
>> "?column?"
>> ) AS
>> SELECT "*SELECT* 1".moo, "*SELECT* 1"."?column?"
>>   WITH NO DATA;
>>
>
> A separate issue is with psql tab-completion:
>
> postgres=# COMMENT ON MATERIALIZED VIEW ^IIS
>
> This should be offering MV names instead of prematurely providing the "IS"
> keyword.
>

Also in doc/src/sgml/ref/alter_materialized_view.sgml:

s/materailized/materialized/


In src/backend/executor/execMain.c:

s/referrenced/referenced/

-- 
Thom

Re: [HACKERS] Passing connection string to pg_basebackup

[Dimitri Fontaine]

Heikki Linnakangas  writes:
> You could still use environment variables and a service file to do it, but
> it's certainly more cumbersome. It clearly should be possible to pass a full
> connection string to pg_basebackup, that's an obvious oversight.

FWIW, +1. I would consider it a bugfix (backpatch, etc).

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] Passing connection string to pg_basebackup

[Amit Kapila]

On Friday, January 18, 2013 5:35 PM Heikki Linnakangas wrote:
> On 18.01.2013 13:41, Amit Kapila wrote:
> > On Friday, January 18, 2013 3:46 PM Heikki Linnakangas wrote:
> >> On 18.01.2013 08:50, Amit Kapila wrote:
> >>> I think currently user has no way to specify TCP keepalive settings
> >> from
> >>> pg_basebackup, please let me know if there is any such existing
> way?
> >>
> >> I was going to say you can just use "keepalives_idle=30" in the
> >> connection string. But there's no way to pass a connection string to
> >> pg_basebackup on the command line! The usual way to pass a
> connection
> >> string is to pass it as the database name, and PQconnect will expand
> >> it,
> >> but that doesn't work with pg_basebackup because it hardcodes the
> >> database name as "replication". D'oh.
> >>
> >> You could still use environment variables and a service file to do
> it,
> >> but it's certainly more cumbersome. It clearly should be possible to
> >> pass a full connection string to pg_basebackup, that's an obvious
> >> oversight.
> >
> > So to solve this problem below can be done:
> > 1. Support connection string in pg_basebackup and mention keepalives
> or
> > connection_timeout
> > 2. Support recv_timeout separately to provide a way to users who are
> not
> > comfortable tcp keepalives
> >
> > a. 1 can be done alone
> > b. 2 can be done alone
> > c. both 1 and 2.
> 
> Right. Let's do just 1 for now. 

I shall fix it as Review comment and update the patch and change the
location from CF-3 to current CF.

> An general application level, non-TCP,
> keepalive message at the libpq level might be a good idea, but that's a
> much larger patch, definitely not 9.3 material.


With Regards,
Amit Kapila.



-- 
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: Patch to compute Max LSN of Data Pages

[Amit kapila]

Please find the rebased Patch for Compute MAX LSN.



There was one compilation error as "undefined reference to XLByteLT " as 
earlier  XLogRecPtr was a structure as
typedef struct XLogRecPtr
{
uint32xlogid;   
 /* log file #, 0 based */
uint32xrecoff;
/* byte offset of location in log file */
} XLogRecPtr;
So in order to compare two LSN, there was one macro as XLByteLT to compare both 
fields.
But now XLogRecPtr  has been changed as just uint64 and so XLByteLT is removed.
So the change done is to replace XLByteLT(*maxlsn, pagelsn) with (*maxlsn < 
pagelsn).

Muhammad, Can you verify if every thing is okay, then this can be marked as 
"Ready for Committer"



With Regards,

Amit Kapila.


pg_computemaxlsn_v5.patch
Description: pg_computemaxlsn_v5.patch

-- 
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] could not create directory "...": File exists

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Don't see what.  The main reason we've not yet attempted a global fix is
> that the most straightforward way (take a new snapshot each time we
> start a new SnapshotNow scan) seems too expensive.  But CREATE DATABASE
> is so expensive that the cost of an extra snapshot there ain't gonna
> matter.

  Patch attached.  Passes the regression tests and our internal testing
  shows that it eliminates the error we were seeing (and doesn't cause
  blocking, which is even better).

  We have a workaround in place for our build system (more-or-less
  "don't do that" approach), but it'd really be great if this was
  back-patched and in the next round of point releases.  Glancing
  through the branches, looks like it should apply pretty cleanly.

Thanks,

Stephen
colordiff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
new file mode 100644
index 1f6e02d..94e1a5d
*** a/src/backend/commands/dbcommands.c
--- b/src/backend/commands/dbcommands.c
*** createdb(const CreatedbStmt *stmt)
*** 131,136 
--- 131,137 
  	int			notherbackends;
  	int			npreparedxacts;
  	createdb_failure_params fparms;
+ 	Snapshot	snapshot;
  
  	/* Extract options from the statement node tree */
  	foreach(option, stmt->options)
*** createdb(const CreatedbStmt *stmt)
*** 535,540 
--- 536,555 
  	RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE | CHECKPOINT_WAIT);
  
  	/*
+ 	 * Take a snapshot to use while scanning through pg_tablespace.  As we
+ 	 * create directories and copy files around, this process could take a
+ 	 * while, so also register that snapshot.
+ 	 *
+ 	 * Traversing pg_tablespace with an MVCC snapshot is necessary to provide
+ 	 * us with a consistent view of the tablespaces that exist.  Using
+ 	 * SnapshotNow here would risk seeing the same tablespace multiple times
+ 	 * or, worse, not seeing a tablespace at all if its tuple is moved around
+ 	 * by other concurrent operations (eg: due to the ACL on the tablespace
+ 	 * being updated).
+ 	 */
+ 	snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 
+ 	/*
  	 * Once we start copying subdirectories, we need to be able to clean 'em
  	 * up if we fail.  Use an ENSURE block to make sure this happens.  (This
  	 * is not a 100% solution, because of the possibility of failure during
*** createdb(const CreatedbStmt *stmt)
*** 551,557 
  		 * each one to the new database.
  		 */
  		rel = heap_open(TableSpaceRelationId, AccessShareLock);
! 		scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
  		while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
  		{
  			Oid			srctablespace = HeapTupleGetOid(tuple);
--- 566,572 
  		 * each one to the new database.
  		 */
  		rel = heap_open(TableSpaceRelationId, AccessShareLock);
! 		scan = heap_beginscan(rel, snapshot, 0, NULL);
  		while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
  		{
  			Oid			srctablespace = HeapTupleGetOid(tuple);
*** createdb(const CreatedbStmt *stmt)
*** 656,661 
--- 671,679 
  	PG_END_ENSURE_ERROR_CLEANUP(createdb_failure_callback,
  PointerGetDatum(&fparms));
  
+ 	/* Free our snapshot */
+ 	UnregisterSnapshot(snapshot);
+ 
  	return dboid;
  }
  


signature.asc
Description: Digital signature

Re: [HACKERS] Teaching pg_receivexlog to follow timeline switches

[Heikki Linnakangas]

On 18.01.2013 06:38, Phil Sorber wrote:

On Tue, Jan 15, 2013 at 9:05 AM, Heikki Linnakangas
  wrote:

Now that a standby server can follow timeline switches through streaming
replication, we should do teach pg_receivexlog to do the same. Patch
attached.


Is it possible to re-use walreceiver code from the backend?

I was thinking that it would actually be very useful to have the whole
replication functionality modularized and in a standalone binary that
could act as a replication proxy and WAL archiver that could run
without all the overhead of an entire PG instance


There's much sense in trying to extract that into a stand-along module. 
src/bin/pg_basebackup/receivelog.c is about 1000 lines of code at the 
moment, and it looks quite different from the corresponding code in the 
backend, because it doesn't have all the backend infrastructure available.


- Heikki


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