Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

2016-02-09 Thread Josh Kupershmidt
On Tue, Feb 9, 2016 at 2:16 PM, Filip Rembiałkowski
 wrote:
> But then it becomes disputable if SQL syntax change makes sense.
>
> ---we had this,
>  NOTIFY channel [ , payload ]
> ---and in this patch we have this
> NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
>  ---  but maybe we should have this?
> NOTIFY channel [ , payload [ , mode ] ]

I think using ALL to mean "don't worry about de-duplication" could be
a bit confusing, especially as there was some interest recently in
supporting wildcard notifications:
http://www.postgresql.org/message-id/52693fc5.7070...@gmail.com

and conceivably we might want to support a way to notify all
listeners, i.e. NOTIFY * as proposed in that thread. If we ever
supported wildcard notifies, ALL may be easily confused to mean "all
channel names".

What about adopting the options-inside-parentheses format, the way
EXPLAIN does nowadays, something like:

NOTIFY (DEDUPLICATE FALSE, MODE IMMEDIATE) mychannel;

Josh


-- 
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] Proposing pg_hibernate

2014-05-30 Thread Josh Kupershmidt
On Tue, May 27, 2014 at 10:01 PM, Gurjeet Singh gurj...@singh.im wrote:

 When the Postgres server is being stopped/shut down, the `Buffer
 Saver` scans the
 shared-buffers of Postgres, and stores the unique block identifiers of
 each cached
 block to the disk. This information is saved under the 
 `$PGDATA/pg_hibernator/`
 directory. For each of the database whose blocks are resident in shared 
 buffers,
 one file is created; for eg.: `$PGDATA/pg_hibernator/2.postgres.save`.

This file-naming convention seems a bit fragile. For example, on my
filesystem (HFS) if I create a database named foo / bar, I'll get a
complaint like:

ERROR:  could not open pg_hibernator/5.foo / bar.save: No such file
or directory

during shutdown.

Josh


-- 
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] Odd uuid-ossp behavior on smew and shearwater

2014-05-29 Thread Josh Kupershmidt
On Wed, May 28, 2014 at 11:23 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Buildfarm critters smew and shearwater are reporting regression test
 failures that suggest that the UUID library can't get a system MAC
 address:
 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=smewdt=2014-05-28%2023%3A38%3A28
 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=shearwaterdt=2014-05-29%2000%3A24%3A32

 I've just committed regression test adjustments to prevent that from
 being a failure case, but I am confused about why it's happening.
 I wouldn't be surprised at not getting a MAC address on a machine that
 lacks any internet connection, but that surely can't describe the
 buildfarm environment.  Are you curious enough to poke into it and
 see what's going on?  It might be useful to strace a backend that's
 trying to execute uuid_generate_v1() and see what the kernel interaction
 looks like exactly.

Here's the result of attaching strace to an idle backend, then running
SELECT uuid_generate_v1(). AFAIR shearwater is a cheaply-hosted OpenVZ
VPS under the hood.

Josh
josh@ease1:~$ strace -p 6818
Process 6818 attached - interrupt to quit
recv(10, Q\0\0\0\37SELECT uuid_generate_v1();\0, 8192, 0) = 32
gettimeofday({1401383296, 920282}, NULL) = 0
gettimeofday({1401383296, 920313}, NULL) = 0
mmap2(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 
0xae80
stat64(/home/josh/runtime/lib/postgresql/uuid-ossp, 0xbfdf87d0) = -1 ENOENT 
(No such file or directory)
stat64(/home/josh/runtime/lib/postgresql/uuid-ossp.so, {st_mode=S_IFREG|0755, 
st_size=46685, ...}) = 0
stat64(/home/josh/runtime/lib/postgresql/uuid-ossp.so, {st_mode=S_IFREG|0755, 
st_size=46685, ...}) = 0
open(/home/josh/runtime/lib/postgresql/uuid-ossp.so, O_RDONLY) = 7
read(7, \177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0p\f\0\0004\0\0\0..., 
512) = 512
fstat64(7, {st_mode=S_IFREG|0755, st_size=46685, ...}) = 0
mmap2(NULL, 16796, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 7, 0) = 
0xae7fb000
mmap2(0xae7ff000, 4096, PROT_READ|PROT_WRITE, 
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 7, 0x3) = 0xae7ff000
close(7)= 0
open(/home/josh/runtime/lib/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file 
or directory)
open(tls/i686/sse2/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or 
directory)
open(tls/i686/sse2/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or 
directory)
open(tls/i686/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or 
directory)
open(tls/i686/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(tls/sse2/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or 
directory)
open(tls/sse2/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(tls/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(tls/libuuid.so.1, O_RDONLY)  = -1 ENOENT (No such file or directory)
open(i686/sse2/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or 
directory)
open(i686/sse2/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(i686/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(i686/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(sse2/cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(sse2/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(cmov/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file or directory)
open(libuuid.so.1, O_RDONLY)  = -1 ENOENT (No such file or directory)
open(/home/josh/runtime/lib/libuuid.so.1, O_RDONLY) = -1 ENOENT (No such file 
or directory)
open(/etc/ld.so.cache, O_RDONLY)  = 7
fstat64(7, {st_mode=S_IFREG|0644, st_size=30628, ...}) = 0
mmap2(NULL, 30628, PROT_READ, MAP_PRIVATE, 7, 0) = 0xae7f3000
close(7)= 0
access(/etc/ld.so.nohwcap, F_OK)  = -1 ENOENT (No such file or directory)
open(/lib/i386-linux-gnu/libuuid.so.1, O_RDONLY) = 7
read(7, 
\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\260\22\0\0004\0\0\0..., 512) 
= 512
fstat64(7, {st_mode=S_IFREG|0644, st_size=18000, ...}) = 0
mmap2(NULL, 20712, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 7, 0) = 
0xae7ed000
mmap2(0xae7f1000, 8192, PROT_READ|PROT_WRITE, 
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 7, 0x3) = 0xae7f1000
close(7)= 0
mprotect(0xae7f1000, 4096, PROT_READ)   = 0
munmap(0xae7f3000, 30628)   = 0
socket(PF_FILE, SOCK_STREAM, 0) = 7
connect(7, {sa_family=AF_FILE, path=/var/run/uuidd/request}, 110) = -1 ENOENT 
(No such file or directory)
access(/usr/sbin/uuidd, X_OK) = -1 ENOENT (No such file or directory)
close(7)= 0
socket(PF_INET, SOCK_DGRAM, IPPROTO_IP) = 7
ioctl(7, SIOCGIFCONF, {96, {{lo, {AF_INET, inet_addr(127.0.0.1)}}, 
{venet0, {AF_INET, inet_addr(127.0.0.2)}}, {venet0:0, {AF_INET, 
inet_addr(198.204.250.34)) = 0
ioctl(7, SIOCGIFHWADDR, {ifr_name=lo, ifr_hwaddr=00:00:00:00:00:00}) = 

Re: [HACKERS] Odd uuid-ossp behavior on smew and shearwater

2014-05-29 Thread Josh Kupershmidt
On Thu, May 29, 2014 at 4:06 PM, Andrew Dunstan and...@dunslane.net wrote:
 On 05/29/2014 02:38 PM, Tom Lane wrote:
 Josh Kupershmidt schmi...@gmail.com writes:
 Interesting.  Looks like you have access only to virtual network
 interfaces, and they report all-zero MAC addresses, which the UUID library
 is smart enough to ignore.  If smew is also in a virtual environment
 then that's probably the explanation.  (There are some other buildfarm
 critters that are reporting MAC addresses with the local-admin bit set,
 which I suspect also means they've got virtual network interfaces, but
 with a different treatment of the what-to-report problem.)

 Almost all my critters run in VMs (all but jacana and bowerbird).

They're not running on OpenVZ, are they? `ifconfig` on shearwater says:

venet0Link encap:UNSPEC  HWaddr
00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
  inet addr:127.0.0.2  P-t-P:127.0.0.2  Bcast:0.0.0.0
Mask:255.255.255.255
  UP BROADCAST POINTOPOINT RUNNING NOARP  MTU:1500  Metric:1
  RX packets:1409294 errors:0 dropped:0 overruns:0 frame:0
  TX packets:1488401 errors:0 dropped:0 overruns:0 carrier:0
  collisions:0 txqueuelen:0
  RX bytes:751149524 (716.3 MiB)  TX bytes:740573200 (706.2 MiB)

venet0:0  Link encap:UNSPEC  HWaddr
00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
  inet addr:198.204.250.34  P-t-P:198.204.250.34
Bcast:0.0.0.0  Mask:255.255.255.255
  UP BROADCAST POINTOPOINT RUNNING NOARP  MTU:1500  Metric:1

and it seems this all-zeros MAC address is a common
(mis-?)configuration on OpenVZ:

https://github.com/robertdavidgraham/masscan/issues/43
http://stackoverflow.com/questions/5838225/how-do-i-get-a-default-gridgain-node-in-openvz-discover-other-nodes-on-the-same
http://forum.openvz.org/index.php?t=msggoto=8117


-- 
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: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-10-22 Thread Josh Kupershmidt
On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt schmi...@gmail.com wrote:
 Also, Pavel, this patch is still listed as 'Needs Review' in the CF
 app, but I haven't seen a response to the concerns in my last message.

It looks like this patch has been imported into the 2013-11 CF [1] and
marked Needs Review. I looked at the version of the patch pointed to
in that CF entry back in July, and noted [2] several problems that
still seemed to be present in the patch, for which I never saw a
followup from Pavel.  IMO this patch should have gotten marked
Returned with Feedback pending a response from Pavel.

Josh

[1] https://commitfest.postgresql.org/action/patch_view?id=1174
[2] 
http://www.postgresql.org/message-id/CAK3UJREth9DVL5U7ewOLQYhXF7EcV5BABFE+pzPQjkPfqbW=v...@mail.gmail.com


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


Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-10-14 Thread Josh Kupershmidt
On Thu, Oct 10, 2013 at 12:54 PM, Andrew Dunstan and...@dunslane.net wrote:

 This thread seems to have gone cold, but I'm inclined to agree with Pavel.
 If the table doesn't exist, neither does the trigger, and the whole point of
 the 'IF EXISTS' variants is to provide the ability to issue DROP commands
 that don't fail if their target is missing.

+1 from me as well.

Also, Pavel, this patch is still listed as 'Needs Review' in the CF
app, but I haven't seen a response to the concerns in my last message.

Josh


-- 
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_restore multiple --function options

2013-09-05 Thread Josh Kupershmidt
On Tue, Aug 27, 2013 at 1:14 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
 Assuming no objections, I'll apply the attached patch to 9.3 and master
 later tonight.

Just a little stylistic nitpick: could we pluralize the --help outputs
for the modified options so that they make clear that multiple
specifications are supported. E.g. restore named index(es) instead
of just restore named index. The last round of such changes tried to
make such --help tweaks, it would be nice to keep 'em consistent.

Josh


-- 
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: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-07-10 Thread Josh Kupershmidt
On Fri, Jul 5, 2013 at 12:16 PM, Pavel Stehule pavel.steh...@gmail.com wrote:

 I am sending a patch that removes strict requirements for DROP IF
 EXISTS statements. This behave is similar to our ALTER IF EXISTS
 behave now.

+1 for this idea. But this patch should be treated as a separate issue
from the use of IF EXISTS in pg_dump/pg_restore, right? If so, I
suggest starting a new thread about this patch to make reviewing
easier.

Josh


-- 
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: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-07-10 Thread Josh Kupershmidt
On Tue, Jul 2, 2013 at 5:39 AM, Pavel Stehule pavel.steh...@gmail.com wrote:

 2013/3/8 Josh Kupershmidt schmi...@gmail.com:
 On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule pavel.steh...@gmail.com 
 wrote:
 2013/3/8 Josh Kupershmidt schmi...@gmail.com:

 Cool. I think it would also be useful to check that --clean may only
 be used with --format=p to avoid any confusion there. (This issue
 could be addressed in a separate patch if you'd rather not lard this
 patch.)

 no

 some people - like we in our company would to use this feature for
 quiet and strict mode for plain text format too.

 enabling this feature has zero overhead so there are no reason block
 it anywhere.

 I'm not sure I understand what you're getting at, but maybe my
 proposal wasn't so clear. Right now, you can specify --clean along
 with -Fc to pg_dump, and pg_dump will not complain even though this
 combination is nonsense. I am proposing that pg_dump error out in this
 case, i.e.

   $ pg_dump -Fc --file=test.dump --clean -d test
   pg_dump: option --clean only valid with plain format dump

 Although this lack of an error a (IMO) misfeature of existing pg_dump,
 so if you'd rather leave this issue aside for your patch, that is
 fine.


 I tested last patch and I am thinking so this patch has sense for
 custom format too

 [postgres@localhost ~]$ /usr/local/pgsql/bin/pg_dump   --clean -t a
 -Fc postgres  dump
 [postgres@localhost ~]$ psql -c drop table a
 DROP TABLE
 [postgres@localhost ~]$ pg_restore --clean -d postgres dump
 pg_restore: [archiver (db)] Error while PROCESSING TOC:
 pg_restore: [archiver (db)] Error from TOC entry 171; 1259 16462 TABLE
 a postgres
 pg_restore: [archiver (db)] could not execute query: ERROR:  table a
 does not exist
 Command was: DROP TABLE public.a;

 WARNING: errors ignored on restore: 1

 [postgres@localhost ~]$ /usr/local/pgsql/bin/pg_dump  --if-exist
 --clean -t a -Fc postgres  dump
 [postgres@localhost ~]$ psql -c drop table a
 DROP TABLE
 [postgres@localhost ~]$ pg_restore --clean -d postgres dump

 So limit for plain format is not too strict

I'm not sure I understand what you're proposing here, but for the
record I really don't think it's a good idea to encourage the use of
  pg_dump -Fc --clean ...

Right now, we don't error out on this combination of command-line
options, but the --clean is effectively a no-op; you have to tell
pg_restore to use --clean. IMO, this is basically how it should be:
pg_dump, at least in custom-format, is preserving the contents of your
database with the understanding that you will use pg_restore wish to
restore from this dump in a variety of possible ways. Putting
restrictions like --clean into the custom-format dump file just makes
that dump file less useful overall.

Although it's still not clear to me why the examples you showed above
used *both* `pg_dump -Fc --clean ...` and `pg_restore --clean ...`
together. Surely the user should be specifying this preference in only
one place?

Josh


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


[HACKERS] tab-completion for \lo_import

2013-07-10 Thread Josh Kupershmidt
Hi all,
Is there any reason not to tab-complete the local filename used by the
\lo_import command? Small patch to do so attached.
Josh


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


[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-07-10 Thread Josh Kupershmidt
On Tue, Jul 2, 2013 at 7:47 AM, Pavel Stehule pavel.steh...@gmail.com wrote:
 Hello

 remastered patch

 still there is a issue with dependencies

Several of the issues from my last review [1] seem to still be present
in this patch, such as review notes #1 and #4.

And as discussed previously, I think that the --clean option belongs
solely with pg_restore for custom-format dumps. The way the patch
handles this is rather confusing, forcing the user to do:

  $  pg_dump -Fc --clean --if-exists --file=backup.dump ...
and then:
  $  pg_restore --clean ... backup.dump (without --if-exists)

to get the desired behavior.

Josh

[1] 
http://www.postgresql.org/message-id/CAK3UJRG__4=+f46xamiqa80f_-bqhjcpfwyp8g8fpspqj-j...@mail.gmail.com


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


Re: [HACKERS] vacuumlo - use a cursor

2013-07-07 Thread Josh Kupershmidt
On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan and...@dunslane.net wrote:
 vacuumlo is rather simpleminded about dealing with the list of LOs to be
 removed - it just fetches them as a straight resultset. For one of my our
 this resulted in an out of memory condition.

Wow, they must have had a ton of LOs. With about 2M entries to pull
from vacuum_l, I observed unpatched vacuumlo using only about 45MB
RES. Still, the idea of using a cursor for the main loop seems like a
reasonable idea.

 The attached patch tries to
 remedy that by using a cursor instead. If this is wanted I will add it to
 the next commitfest. The actualy changes are very small - most of the patch
 is indentation changes due to the introduction of an extra loop.

I had some time to review this, some comments about the patch:

1. I see this new compiler warning:

vacuumlo.c: In function ‘vacuumlo’:
vacuumlo.c:306:5: warning: format ‘%lld’ expects argument of type
‘long long int’, but argument 4 has type ‘long int’ [-Wformat]

2. It looks like the the patch causes all the intermediate result-sets
fetched from the cursor to be leaked, rather negating its stated
purpose ;-) The PQclear() call should be moved up into the main loop.
With this fixed, I confirmed that vacuumlo now consumes a negligible
amount of memory when chewing through millions of entries.

3. A few extra trailing whitespaces were added.

4. The FETCH FORWARD count comes from transaction_limit. That seems
like a good-enough guesstimate, but maybe a comment could be added to
rationalize?

Some suggested changes attached with v2 patch (all except #4).

Josh


vacuumlo-cursor.v2.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] fixing pg_ctl with relative paths

2013-07-01 Thread Josh Kupershmidt
On Thu, Jun 27, 2013 at 11:47 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt schmi...@gmail.com wrote:
 On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Though this is a corner case, the patch doesn't seem to handle properly the 
 case
 where -D appears as other option value, e.g., -k option value, in
 postmaster.opts
 file.

 Could I see a command-line example of what you mean?

 postmaster -k -D, for example. Of course, it's really a corner case :)

Oh, I see. I was able to trip up strip_datadirs() with something like

$ PGDATA=/my/data/ postmaster -k -D -S 100 
$ pg_ctl -D /my/data/ restart

that example causes pg_ctl to fail to start the server after stopping
it, although perhaps you could even trick the server into starting
with the wrong options. Of course, similar problems exists today in
other cases, such as with the relative paths issue this patch is
trying to address, or a datadir containing embedded quotes.

I am eager to see the relative paths issue fixed, but maybe we need to
bite the bullet and sort out the escaping of command-line options in
the rest of pg_ctl first, so that a DataDir like /tmp/here's a \
quote can consistently be used by pg_ctl {start|stop|restart} before
we can fix this wart.

Josh


-- 
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] fixing pg_ctl with relative paths

2013-06-26 Thread Josh Kupershmidt
On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu haribabu.ko...@huawei.com wrote:
 On June 26, 2013 5:02 AM Josh Kupershmidt wrote:
Thanks for the feedback. Attached is a rebased version of the patch with
 the two small issues noted fixed.

 The following description in the document of pg_ctl needs to be modified?

 restart might fail if relative paths specified were specified on
 the command-line during server start.

Right, that caveat could go away.

 +#define DATADIR_SPEC   \-D\ \
 +
 +   datadir = strstr(post_opts, DATADIR_SPEC);

 Though this is a corner case, the patch doesn't seem to handle properly the 
 case
 where -D appears as other option value, e.g., -k option value, in
 postmaster.opts
 file.

Could I see a command-line example of what you mean?

 Just idea to work around that problem is to just append the specified -D 
 option
 and value to post_opts. IOW, -D option and value appear twice in post_opts.
 In this case, posteriorly-located ones are used in the end. Thought?

Hrm, I think we'd have to be careful that postmaster.opts doesn't
accumulate an additional -D specification with every restart.

Josh


-- 
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] fixing pg_ctl with relative paths

2013-06-25 Thread Josh Kupershmidt
On Tue, Jun 25, 2013 at 2:28 AM, Hari Babu haribabu.ko...@huawei.com wrote:
 Please find the review of the patch.

Thank you for reviewing!

 Code Review:
 
 +if (orig_post_opts) {
 +post_opts = strip_datadirs(orig_post_opts);
 +}

 No need of {} as the only one statement block is present in the if block.

OK.

 + free(tmp);

 The above statement can be moved inside the if (*(trailing_quote + 1) !=
 '\0') {
 where it's exact usage is present.

Right.

 Testing:
 
 I tested this feature with different postmaster options and database folder
 names, found no problem.


 The database folder with quotes present in it is any way having problems
 with pg_ctl.
 I feel the strip_datadirs() function header explanation is providing good
 understanding.
 Overall the patch is good. It makes the pg_ctl restart functionality works
 well.

Thanks for the feedback. Attached is a rebased version of the patch
with the two small issues noted fixed.

Josh


pgctl_paths.v02.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] [9.4 CF 1] The Commitfest Slacker List

2013-06-24 Thread Josh Kupershmidt
On Mon, Jun 24, 2013 at 12:57 PM, Josh Berkus j...@agliodbs.com wrote:
 Actually, every submitter on that list -- including Maciej -- was sent a
 personal, private email a week ago.  A few (3) chose to take the
 opportunity to review things, or promised to do so, including a brand
 new Chinese contributor who needed help with English to review his
 patch.  The vast majority chose not to respond to my email to them at
 all.  When private email fails, the next step is public email.

Hrm, I'm on the slackers list, and I didn't see an email directed to
me from JB in the last week about the CF.

Anyway, I am hoping to take at least one patch this CF, though the
recent review it within 5 days or else policy combined with a ton of
my own work has kept me back so far.

Josh


-- 
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_filedump 9.3: checksums (and a few other fixes)

2013-06-24 Thread Josh Kupershmidt
On Tue, Jun 18, 2013 at 12:42 PM, Jeff Davis pg...@j-davis.com wrote:
 Attached a new diff for pg_filedump that makes use of the above change.

 I'm not sure what the resolution of Alvaro's concern was, so I left the
 flag reporting the same as the previous patch.

This patch is in the current CommitFest, does it still need to be
reviewed? If so, I notice that the version in pgfoundry's CVS is
rather different than the version the patch seems to have been built
against (presumably the pg_filedump-9.2.0.tar.gz release), and
conflicts in several places with cvs tip.

Also, would anyone be willing to convert this repository to git and
post it on github or similar? pgfoundry is becoming increasingly
difficult to use, for instance the 'Browse CVS Repository' link for
pg_filedump and other projects is broken[1] and apparently has been
for months[2], not to mention the general crumminess of using CVS [3].

Josh

[1] http://pgfoundry.org/scm/browser.php?group_id=1000541
[2] 
http://pgfoundry.org/forum/forum.php?thread_id=15554forum_id=44group_id=113
[3] Since the pgfoundry cvs server apparently doesn't support the 'ls'
command, you might need to know this to know which module name to
check out: cvs -d
:pserver:anonym...@cvs.pgfoundry.org:/cvsroot/pgfiledump checkout
pg_filedump


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


[HACKERS] isolationtester and 'specs' subdirectory

2013-06-20 Thread Josh Kupershmidt
Hi all,
I have a Debian machine with gcc 4.7.2-5 where make check-world fails
in the isolation check, like so:

...
make[2]: Leaving directory `/home/josh/src/postgresql/src/test/regress'
make -C isolation check

[snip]

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -I.
-I../../../src/interfaces/libpq -I./../regress -I../../../src/include
-D_GNU_SOURCE   -c -o isolation_main.o isolation_main.c
gcc: error: ./specs: Is a directory
make[2]: *** [isolation_main.o] Error 1
...

I eventually tracked down the cause of this failure to a trailing ':'
in my $LIBRARY_PATH, which causes gcc to look inside the current
directory for a 'specs' file [1] among other things. Although I
probably don't need that trailing ':', it seems like we should avoid
naming this directory 'specs' nonetheless to avoid confusion with gcc.

Renaming the 'specs' directory to something like 'isolation_specs' and
adjusting isolation_main.c accordingly lets me pass `make
check-world`. Proposed patch attached.

Josh

[1] http://gcc.gnu.org/ml/gcc-help/2010-05/msg00292.html


rename_specs.diff.gz
Description: GNU Zip compressed data

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


[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-06-17 Thread Josh Kupershmidt
On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule pavel.steh...@gmail.com wrote:

 I'll see - please, stay tuned to 9.4 first commitfest

Hi Pavel,
Just a reminder, I didn't see this patch in the current commitfest. I
would be happy to spend some more time reviewing if you wish to pursue
the patch.

Josh


-- 
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_dump/restore syntax checking bug?

2013-03-22 Thread Josh Kupershmidt
On Fri, Mar 22, 2013 at 9:35 PM, Joshua D. Drake j...@commandprompt.com wrote:

 postgres@jd-laptop:~$ pg_restore -d test -P 'by(),hello()' foo.sqlc

Note, the pg_restore doc makes no mention of trying to squeeze
multiple function prototypes in a single argument you've done here, or
of using multiple -P flags.

 It appears we need better syntax checking.

Can't really argue with this. But if you think these pg_restore
examples are bad, try this gem:
  reindexdb --table='foo; ALTER ROLE limited WITH superuser'

Josh


-- 
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] Ignore invalid indexes in pg_dump

2013-03-20 Thread Josh Kupershmidt
On Wed, Mar 20, 2013 at 2:00 AM, Simon Riggs si...@2ndquadrant.com wrote:
 On 20 March 2013 02:51, Michael Paquier michael.paqu...@gmail.com wrote:

 If failures happen with CREATE INDEX CONCURRENTLY, the system will be let
 with invalid indexes. I don't think that the user would like to see invalid
 indexes of
 an existing system being recreated as valid after a restore.
 So why not removing from a dump invalid indexes with something like the
 patch
 attached?
 This should perhaps be applied in pg_dump for versions down to 8.2 where
 CREATE
 INDEX CONCURRENTLY has been implemented?

 Invalid also means currently-in-progress, so it would be better to keep them 
 in.

For invalid indexes which are left hanging around in the database, if
the index definition is included by pg_dump, it will likely cause pain
during the restore. If the index build failed the first time and
hasn't been manually dropped and recreated since then, it's a good bet
it will fail the next time. Errors during restore can be more than
just a nuisance; consider restores with --single-transaction.

And if the index is simply currently-in-progress, it seems like the
expected behavior would be for pg_dump to ignore it anyway. We don't
include other DDL objects which are not yet committed while pg_dump is
running.

Josh


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


Re: [HACKERS] Add some regression tests for SEQUENCE

2013-03-18 Thread Josh Kupershmidt
On Mon, Mar 18, 2013 at 3:10 PM, Robins Tharakan thara...@gmail.com wrote:
 Hi,

 Please find an updated patch (reworked on the names of SEQUENCES / ROLES /
 SCHEMA etc.)
 Takes code-coverage of 'make check' for SEQUENCE to ~95%.

There is a typo difference between sequence.out and sequence.sql
causing the test to fail:

+-- Should fail since seq5 shouldn't exist
...
+-- Should fail since seq5 shouldn't exit

Josh


-- 
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: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-03-08 Thread Josh Kupershmidt
On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule pavel.steh...@gmail.com wrote:
 2013/3/8 Josh Kupershmidt schmi...@gmail.com:

 Cool. I think it would also be useful to check that --clean may only
 be used with --format=p to avoid any confusion there. (This issue
 could be addressed in a separate patch if you'd rather not lard this
 patch.)

 no

 some people - like we in our company would to use this feature for
 quiet and strict mode for plain text format too.

 enabling this feature has zero overhead so there are no reason block
 it anywhere.

I'm not sure I understand what you're getting at, but maybe my
proposal wasn't so clear. Right now, you can specify --clean along
with -Fc to pg_dump, and pg_dump will not complain even though this
combination is nonsense. I am proposing that pg_dump error out in this
case, i.e.

  $ pg_dump -Fc --file=test.dump --clean -d test
  pg_dump: option --clean only valid with plain format dump

Although this lack of an error a (IMO) misfeature of existing pg_dump,
so if you'd rather leave this issue aside for your patch, that is
fine.

Josh


-- 
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: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013-03-07 Thread Josh Kupershmidt
[Moving to -hackers]

On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule pavel.steh...@gmail.com wrote:

 so

 * --conditional-drops replaced by --if-exists

Thanks for the fixes, I played around with the patch a bit. I was sort
of expecting this example to work (after setting up the regression
database with `make installcheck`)

  pg_dump --clean --if-exists -Fp -d regression --file=regression.sql
  createdb test
  psql -v ON_ERROR_STOP=1 --single-transaction -d test -f regression.sql

But it fails, first at:
  ...
  DROP TRIGGER IF EXISTS tsvectorupdate ON public.test_tsvector;
  ERROR:  relation public.test_tsvector does not exist

This seems like a shortcoming of DROP TRIGGER ... IF EXISTS, and it
looks like DROP RULE ... IF EXISTS has the same problem. I recall DROP
... IF EXISTS being fixed recently for not to error out if the schema
specified for the object does not exist, and ISTM the same arguments
could be made in favor of fixing DROP TRIGGER/TABLE ... IF EXISTS not
to error out if the table doesn't exist.

Working further through the dump of the regression database, these
also present problems for --clean --if-exists dumps:

  DROP CAST IF EXISTS (text AS public.casttesttype);
  ERROR:  type public.casttesttype does not exist

  DROP OPERATOR IF EXISTS public.% (point, widget);
  ERROR:  type widget does not exist

  DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget);
  ERROR:  type widget does not exist

I'm not sure whether DROP CAST/OPERATOR/FUNCTION IF EXISTS should be
more tolerant of nonexistent types, of if the mess could perhaps be
avoided by dump reordering.

Note, this usability problem affects unpatched head as well:

  pg_dump -Fc -d regression --file=regression.dump
  pg_restore --clean -1 -d regression regression.dump
  ...
  pg_restore: [archiver (db)] could not execute query: ERROR:  type
widget does not exist
  Command was: DROP FUNCTION public.widget_out(widget);

(The use here is a little different than the first example above, but
I would still expect this case to work.) The above problems with IF
EXISTS aren't really a problem of the patch per se, but IMO it would
be nice to straighten all the issues out together for 9.4.

 * -- additional check, available only with -c option

Cool. I think it would also be useful to check that --clean may only
be used with --format=p to avoid any confusion there. (This issue
could be addressed in a separate patch if you'd rather not lard this
patch.)

Some comments on the changes:

1. There is at least one IF EXISTS check missing from pg_dump.c, see
for example this statement from a dump of the regression database with
--if-exists:

ALTER TABLE public.nv_child_2010 DROP CONSTRAINT nv_child_2010_d_check;

2. Shouldn't pg_restore get --if-exists as well?

3.
+   printf(_(  --if-exists  don't report error if
cleaned object doesn't exist\n));

This help output bleeds just over our de facto 80-character limit.
Also contractions seem to be avoided elsewhere. It's a little hard to
squeeze a decent explanation into one line, but perhaps:

  Use IF EXISTS when dropping objects

would be better. The sgml changes could use some wordsmithing and
grammar fixes. I could clean these up for you in a later version if
you'd like.

4. There seem to be spurious whitespace changes to the function
prototype and declaration for _printTocEntry.

That's all I've had time for so far...

Josh


-- 
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] bugfix: --echo-hidden is not supported by \sf statements

2013-03-04 Thread Josh Kupershmidt
On Wed, Feb 27, 2013 at 12:09 PM, Stephen Frost sfr...@snowman.net wrote:
 * Pavel Stehule (pavel.steh...@gmail.com) wrote:
 I don't agree so it works well - you cannot use short type names is
 significant issue

 This is for psql.  In what use-case do you see that being a serious
 limitation?

 I might support having psql be able to fall-back to checking if the
 function name is unique (or perhaps doing that first before going on to
 look at the function arguments) but I don't think this should all be
 punted to the backend where only 9.3+ would have any real support for a
 capability which already exists in other places and should be trivially
 added to these.

Since time is running short for discussion of 9.3:

I still think this patch is an improvement over the status quo, and is
committable as-is. Yes, the patch doesn't address the existing
ugliness with minimal_error_message() and sidestepping PSQLexec(), but
at least it fixes the --echo-hidden behavior, and the various other
issues may be handled separately.

Josh


-- 
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] bugfix: --echo-hidden is not supported by \sf statements

2013-03-04 Thread Josh Kupershmidt
On Mon, Mar 4, 2013 at 11:39 AM, Stephen Frost sfr...@snowman.net wrote:
 Josh,

 * Josh Kupershmidt (schmi...@gmail.com) wrote:
 I still think this patch is an improvement over the status quo, and is
 committable as-is. Yes, the patch doesn't address the existing
 ugliness with minimal_error_message() and sidestepping PSQLexec(), but
 at least it fixes the --echo-hidden behavior, and the various other
 issues may be handled separately.

 Which patch, exactly, are you referring to wrt this..?  I'm less than
 convinced that it's in a committable state, but I'd like to make sure
 that we're talking about the same thing here...

Sorry, this second version posted by Pavel:
http://www.postgresql.org/message-id/cafj8prb3-tov5s2dcgshp+vedyk9s97d7hn7rdmmw9ztrj-...@mail.gmail.com

Josh


-- 
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] bugfix: --echo-hidden is not supported by \sf statements

2013-03-04 Thread Josh Kupershmidt
On Mon, Mar 4, 2013 at 11:54 AM, Stephen Frost sfr...@snowman.net wrote:
 Yeah, no, I don't think we should go in this direction.  The whole
 TraceQuery thing is entirely redundant to what's already there and which
 should have been used from the beginning.  This would be adding on to
 that mistake instead of fixing it.

 If we're going to fix it, let's fix it correctly.

Fair enough. I thought the little extra ugliness was stomachable, but
I'm willing to call this patch Returned with Feedback for now.

Josh


-- 
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] bugfix: --echo-hidden is not supported by \sf statements

2013-02-19 Thread Josh Kupershmidt
On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
 2013/1/14 Tom Lane t...@sss.pgh.pa.us:
 Well, fine, but then it should fix both of them and remove
 minimal_error_message altogether.  I would however suggest eyeballing
 what happens when you try \ef nosuchfunction (with or without -E).
 I'm pretty sure that the reason for the special error handling in these
 functions is that the default error reporting was unpleasant for this
 use-case.

 so I rewrote patch

 still is simple

 On the end I didn't use PSQLexec - just write hidden queries directly
 from related functions - it is less invasive

Sorry for the delay, but I finally had a chance to review this patch.
I think the patch does a good job of bringing the behavior of \sf and
\ef in-line with the other meta-commands as far as --echo-hidden is
concerned.

About the code changes:

The bulk of the code change comes from factoring TraceQuery() out of
PSQLexec(), so that \ef and \sf may make use of this query-printing
without going through PSQLexec(). And not using PSQLexec() lets us
avoid a somewhat uglier error message like:

  ERROR:  function nonexistent does not exist
  LINE 1: SELECT 'nonexistent'::pg_catalog.regproc::pg_catalog.oid

Tom suggested removing minimal_error_message() entirely, which would
be nice if possible. It is a shame that \sf nonexistent produces a
scary-looking ERROR:  message (and would cause any in-progress
transaction to need a rollback) while the other informational
metacommands do not. Actually, the other metacommands are not entirely
consistent with each other; compare \dt nonexistent and \df
nonexistent.

It would be nice if the case of a matching function not found by \sf
or \ef could be handled in the same fashion as:

  # \d nonexistent
  Did not find any relation named nonexistent.

though I realize that's not trivial due to the implementation of
lookup_function_oid(). At any rate, this nitpicking about the error
handling in the case that the function is not found is quibbling about
behavior unchanged by the patch. I don't have any complaints about the
patch itself, so I think it can be applied as-is, and we can attack
the error handling issue separately.

Josh


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


[HACKERS] fixing pg_ctl with relative paths

2013-01-22 Thread Josh Kupershmidt
There have been some complaints[1][2] in the past about pg_ctl not
playing nice with relative path specifications for the datadir. Here's
a concise illustration:

  $ mkdir /tmp/mydata/  initdb /tmp/mydata/
  $ cd /tmp/
  $ pg_ctl -D ./mydata/ start
  $ cd /
  $ pg_ctl -D /tmp/mydata/ restart

IMO it's pretty hard to defend the behavior of the last step, where
pg_ctl knows exactly which datadir the user has specified, and
succeeds in stopping the server but not starting it.

Digging into this gripe, a related problem I noticed is that `pg_ctl
... restart` doesn't always preserve the -D $DATADIR argument as the
following comment suggests it should[4]:

  * We could pass PGDATA just in an environment
  * variable but we do -D too for clearer postmaster
  * 'ps' display

Specifically, if one passes in additional -o options, e.g.

  $ pg_ctl -D /tmp/mydata/ -o -N 10 restart

after which postmaster.opts will be missing the -D ... argument
which is otherwise recorded, and the `ps` output is similarly
abridged.

Anyway, Tom suggested[3] two possible ways of fixing the original
gripe, and I went with his latter suggestion,

| for pg_ctl restart to override that
| option with its freshly-derived idea of where the data directory is

mainly so we don't need to worry about the impact of changing the
appearance of postmaster.opts, plus it seems like this code fits
better in pg_ctl.c rather than the postmaster (where the
postmaster.opts file is actually written). The fix seems to be pretty
simple, namely stripping post_opts of the old -D ...  portion and
having the new specification, if any, be used instead. I believe the
attached patch should fix these two infelicities.

Full disclosure: the strip_datadirs() function makes no attempt to
properly handle pathnames containing quotes. There seems to be some,
uh, confusion in the existing code about how these should be handled
already. For instance,

  $ mkdir /tmp/here's a \ quote
  $ initdb /tmp/here's a \ quote

How to successfully start, restart, and stop the server with pg_ctl is
left as an exercise for the reader. So I tried to avoid that can of
worms with this patch, though I'm happy to robustify strip_datadirs()
if we'd like to properly support such pathnames, and there's consensus
on how to standardize the escaping.

Josh

[1] 
http://www.postgresql.org/message-id/caa-alv72o+negjaiphormbqsmztkzayatxrd+c9v60ybmmm...@mail.gmail.com
[2] 
http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6GtsRVm-LtfWfW=g...@mail.gmail.com
[3] http://www.postgresql.org/message-id/27233.1350234...@sss.pgh.pa.us
[4] Note, ps output and postmaster.opts will not include the datadir
specification if you rely solely on the PGDATA environment variable
for pg_ctl


pgctl_paths.v01.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] string escaping in tutorial/syscat.source

2013-01-15 Thread Josh Kupershmidt
On Tue, Jan 15, 2013 at 6:35 PM, Jeff Janes jeff.ja...@gmail.com wrote:

 Do you propose back-patching this?  You could argue that this is a bug in
 9.1 and 9.2.   Before that, they generate deprecation warnings, but do not
 give the wrong answer.

I think that backpatching to 9.1 would be reasonable, though I won't
complain if the fix is only applied to HEAD.

 If it is only to be applied to HEAD, or only to 9.1, 9.2, and HEAD, then
 this part seems to be unnecessary and I think should be removed (setting a
 value to its default is more likely to cause confusion than remove
 confusion):

 SET standard_conforming_strings TO on;

 and the corresponding reset as well.

Well, it may be unnecessary for people who use the modern default
standard_conforming_strings. But some people have kept
standard_conforming_strings=off in recent versions because they have
old code which depends on this. And besides, it seems prudent to make
the dependency explicit, rather than just hoping the user has the
correct setting, and silently giving wrong results if not. Plus being
able to work with old server versions.

 Other than that, it does what it says (fix a broken example), does it
 cleanly, we want this, and I have no other concerns.

 I guess the src/tutorial directory could participate in regression tests, in
 which case this problem would have been detected when introduced, but I
 don't think I can demand that you invent regression tests for a feature you
 are just fixing rather than creating.

Yeah, I don't think tying into the regression tests is warranted here.

Josh


-- 
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] bad examples in pg_dump README

2013-01-08 Thread Josh Kupershmidt
On Mon, Jan 7, 2013 at 8:12 PM, Peter Eisentraut pete...@gmx.net wrote:
 On Sat, 2013-01-05 at 15:34 +0100, Magnus Hagander wrote:
 On Fri, Jan 4, 2013 at 3:36 AM, Josh Kupershmidt schmi...@gmail.com wrote:
  I propose slimming down the pg_dump README, keeping intact the
  introductory notes and details of the tar format.

 Do we need to keep it at all, really? Certainly the introductory part
 is covered in the main documentation already...

 I'd remove it and distribute the remaining information, if any, between
 the source code and the man page.

Here's a patch to do so. After removing the discussed bogus
information, there were only two notes which I still found relevant,
so I stuck those in the appropriate header comments.

I didn't preserve the comment about blank username  group for
tar'ed files, since there are already some comments along these lines
in tarCreateHeader(), and these are postgres not blank nowadays.
The pg_dump/pg_restore man pages seemed to already do a good enough
job of showing usage examples that I didn't find anything worth adding
there.

Josh


pg_dump_README.v2.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] bad examples in pg_dump README

2013-01-05 Thread Josh Kupershmidt
On Sat, Jan 5, 2013 at 7:34 AM, Magnus Hagander mag...@hagander.net wrote:
 On Fri, Jan 4, 2013 at 3:36 AM, Josh Kupershmidt schmi...@gmail.com wrote:
 Do we need to keep it at all, really? Certainly the introductory part
 is covered in the main documentation already...

Pretty much. (I did find note #2 mildly interesting.)

 I believe the tar notes are also out of date. For example, they claim
 that you can't expect pg_restore to work with an uncompressed tar
 format - yet the header in pg_backup_tar.c says that an uncompressed
 tar format is compatible with a directory format dump, and thus you
 *can* use pg_restore.

 (And fwiw,the note about the user should probably go in src/port/ now
 that we moved the tar header creation there a few days ago)

Hrm yeah, so the second paragraph under the tar section can certainly be axed.

 I would suggest we just drop the README file completely. I don't think
 it adds any value at all.

 Any objections to that path? :)

I think that's OK, since there's not much left in that README after
removing the bogus examples, introductory text that's covered
elsewhere, and obsolete second paragraph about the tar format. Perhaps
we could keep the other paragraphs about the tar format, either in the
header comments for pg_backup_tar.c or in src/port/, though?

Oh, and for this comment in pg_backup_tar.c:

|  *  See the headers to pg_backup_files  pg_restore for more
details.

there is no longer a pg_backup_files.c.

Josh


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


[HACKERS] bad examples in pg_dump README

2013-01-03 Thread Josh Kupershmidt
The ./src/bin/pg_dump README contains several inoperable examples.
First, this suggestion:

|   or to list tables:
|
|   pg_restore backup-file --table | less

seems bogus, i.e. the --table option requires an argument specifing
which table to restore, and does not list tables. Next, this
suggested command:

|  pg_restore backup-file -l --oid --rearrange | less

hasn't worked since 7.4 or thereabouts, since we don't have
--rearrange or --oid anymore. (I'm not sure we ever had --oid, maybe
it was supposed to be --oid-order?). Then, three examples claim we
support a --use flag, e.g.

|  pg_restore backup.bck --use=toc.lis  script.sql

where presumably --use-list was meant. Further little gripes with
this README include mixed use of tabs and spaces for the examples, and
lines running over the 80-column limit which at least some of our
other READMEs seem to honor.

I started out attempting to fix up the README, keeping the original
example commands intact, but upon further reflection I think the
examples of dumping, restoring, and selective-restoring in that REAMDE
don't belong there anyway. There are already better examples of such
usage in the pg_dump/pg_restore documentation pages, and IMO that's
where such generally-useful usage information belongs.

I propose slimming down the pg_dump README, keeping intact the
introductory notes and details of the tar format.

Josh


pg_dump_README.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] discarding duplicate indexes

2012-12-20 Thread Josh Kupershmidt
On Thu, Dec 20, 2012 at 1:26 AM, Gavin Flower
gavinflo...@archidevsys.co.nz wrote:
 On 20/12/12 14:57, Josh Kupershmidt wrote:

 CREATE TABLE test (id int);
 CREATE INDEX test_idx1 ON test (id);
 CREATE INDEX test_idx2 ON test (id);

 I initially misread your example code, but after I realised my mistake, I
 thought of an alternative scenario that might be worth considering.

 CREATE TABLE test (id int, int sub, text payload);
 CREATE INDEX test_idx1 ON test (id, sub);
 CREATE INDEX test_idx2 ON test (id);


 Now test_idx2 is logically included in test_idx1, but if the majority of
 transactions only query on id, then test_idx2 would be more better as it
 ties up less RAM

Well, this situation works without any LIKE ... INCLUDING INDEXES
surprises. If you
  CREATE TABLE test_copycat (LIKE test INCLUDING INDEXES);

you should see test_copycat created with both indexes, since
indexParams is considered for this deduplicating.

Josh


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


[HACKERS] discarding duplicate indexes

2012-12-19 Thread Josh Kupershmidt
I recently came across a scenario like this (tested on git head):


CREATE TABLE test (id int);
CREATE INDEX test_idx1 ON test (id);
CREATE INDEX test_idx2 ON test (id);

CREATE TABLE test_copycat (LIKE test INCLUDING ALL);
\d test_copycat


Why do we end up with only one index on test_copycat? The culprit
seems to be transformIndexConstraints(), which explains:

   * Scan the index list and remove any redundant index specifications. This
   * can happen if, for instance, the user writes UNIQUE PRIMARY KEY. A
   * strict reading of SQL92 would suggest raising an error instead, but
   * that strikes me as too anal-retentive. - tgl 2001-02-14

and this code happily throws out the second index statement in this
example, since its properties are identical to the first. (Side note:
some index properties, such as tablespace specification and comment,
are ignored when determining duplicates). This behavior does seem like
a minor POLA violation to me -- if we do not forbid duplicate indexes
on the original table, it seems surprising to do so silently with
INCLUDING INDEXES.

There was consideration of similar behavior when this patch was
proposed[1], so perhaps the behavior is as-designed, and I guess no
one else has complained. IMO this behavior should at least be
documented under the LIKE source_table section of CREATE TABLE's doc
page.

Josh

[1] http://archives.postgresql.org/pgsql-patches/2007-07/msg00173.php


-- 
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 for checking file parameters to psql before password prompt

2012-12-18 Thread Josh Kupershmidt
On Sun, Dec 2, 2012 at 4:37 AM, Alastair Turner b...@ctrlf5.co.za wrote:
 Patch for the changes discussed in
 http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
 attached (eventually ...)

 In summary: If the input file (-f) doesn't exist or the ouput or log
 files (-o and -l) can't be created psql exits before prompting for a
 password.

I assume you meant -L instead of -l here for specifying psql's log
file. I don't think that the inability to write to psql's log file
should be treated as a fatal error, especially since it is not treated
as such by the current code:

  $ psql test -L /tmp/not_allowed
  psql: could not open log file /tmp/not_allowed: Permission denied
  [... proceeds to psql prompt from here ...]

and the user (or script) may still usefully perform his work. Whereas
with your patch:

  $ psql test -L /tmp/not_allowed
  psql: could not open log file /tmp/not_allowed: Permission denied
  $

And IMO the same concern applies to the query results file, -o.
Although +1 for the part about having psql exit early if the input
filename does not exist, since psql already bails out in this case,
and there is nothing else to be done in such case.

Josh


-- 
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] Multiple --table options for other commands

2012-12-13 Thread Josh Kupershmidt
On Thu, Dec 13, 2012 at 6:05 AM, Karl O. Pinc k...@meme.com wrote:
 On 12/13/2012 12:35:14 AM, Karl O. Pinc wrote:

 On 12/12/2012 11:04:53 PM, Josh Kupershmidt wrote:
  On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc k...@meme.com wrote:
   On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
   On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc k...@meme.com
  wrote:
I believe you need ellipses behind --table in the syntax
  summaries
of the command reference docs.


   Yes.  I see.  I didn't look at all the command's reference pages
   but did happen to look at clusterdb, which does have --table
   in the syntax summary.  I just checked and you need to fix
   clusterdb, reindexdb, and vacuumdb.
 
  OK, I made some changes to the command synopses for these three
  commands.

 I want the ... outside the square braces, because the --table (or -t)
 must repeat for each table specified.  Like:

 vacuumdb [connection-option...] [option...] [ --table | -t table
 [( column [,...] )] ]... [dbname]

 reindexdb [connection-option...] [ --table | -t table ]... [ --index
 |

 -i index ]... [dbname]

 Have you had trouble getting this to work?

 Perhaps arg choice=optarg rep=repeat ... would work?

Well, I fooled around with the SGML for a bit and came up with
something which has the ellipses in brackets:

  [ --table | -t table ] [...]

which I like a little better than not having the second set of
brackets. New version attached.

Josh


multiple_tables.v04.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] Multiple --table options for other commands

2012-12-13 Thread Josh Kupershmidt
On Thu, Dec 13, 2012 at 7:24 PM, Karl O. Pinc k...@meme.com wrote:

 The problem is that the pg man pages would then have a
 syntax different from all the other man pages in the system,
 which all have ... outside of square braces.
 See man cat, e.g.

FWIW, `man cat` on my OS X machine has synopsis:

   cat [-benstuv] [file ...]

though I see:

   cat [OPTION] [FILE]...

on Debian.

 (I wonder if the problem is because the style sheets
 have been tweaked to work well with sql?)

 Because I don't see the traditional man page ellipsis syntax
 anywhere in the pg docs, and because all the pg
 client command line programs that use repeating arguments
 all have a simplified syntax summary with just [options ...]
 or some such, I suspect that there may be a problem
 putting the ellipsis outside of the square braces.

Yeah, I tried a few different ways and didn't manage to get:
 [ --table | -t table ] ...

 It would be nice if we could get some guidance from someone
 more familiar with the pg docbook stylesheets.

 As a fallback I'd do to the clusterdb, reindexdb, and vacuumdb
 syntax summaries what was done to the pg_dump and pg_restore
 syntax summaries.  Remove all the detail.  This is sucky,
 and _still_ leaves the reference pages with a syntax summary syntax
 that differs from regular man pages, but because the text
 is relatively information free nobody notices.

That should be how the v2 patch has it.

 My inclination, since you can't make it work
 and we don't seem to be getting any help here,
 is to remove all the detail in the syntax summaries,
 push it through to a committer, and get some feedback that way.

If someone out there feels that the formatting of these commands'
synopses should look like:
 [ --table | -t table ] ...

and knows how to massage the SGML to get that, I'm happy to
accommodate the change. Otherwise, I think either the v4 or v2 patch
should be acceptable.

Josh


-- 
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] Multiple --table options for other commands

2012-12-13 Thread Josh Kupershmidt
On Thu, Dec 13, 2012 at 9:03 PM, Karl O. Pinc k...@meme.com wrote:
 My brain seems to have turned itself on.  I went and (re)read
 the docbook manual and was able to come up with this,
 which works:


arg choice=plain rep=repeatarg choice=opt
  group choice=plain
arg choice=plainoption--table/option/arg
arg choice=plainoption-t/option/arg
  /group
  replaceabletable/replaceable /arg/arg

 Yay! (indentation et-al aside)

That does seem to work, thanks for figuring out the syntax.

 Sorry to be so persnickety, and unhelpful until now.
 It seemed like it should be doable, but something
 was going wrong between keyboard and chair.  I guess
 I should be doing this when better rested.

No problem, here is v5 with changed synopses.

Josh


multiple_tables.v5.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] Multiple --table options for other commands

2012-12-12 Thread Josh Kupershmidt
On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc k...@meme.com wrote:
 On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
 On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc k...@meme.com wrote:
  I believe you need ellipses behind --table in the syntax summaries
  of the command reference docs.

 Hrm, I was following pg_dump's lead here for the .sgml docs, and
 didn't see anywhere that pg_dump makes the multiple --table syntax
 explicit other than in the explanatory text underneath the option.

 Yes.  I see.  I didn't look at all the command's reference pages
 but did happen to look at clusterdb, which does have --table
 in the syntax summary.  I just checked and you need to fix
 clusterdb, reindexdb, and vacuumdb.

OK, I made some changes to the command synopses for these three
commands. For some reason, reindexdb.sgml was slightly different from
the other two, in that the --table and --index bits were underneath a
group node instead of arg. I've changed that one to be like the
other two, and made them all have:
 arg choice=opt rep=repeat

to include the ellipses after the table -- I hope that matches what
you had in mind.

Josh


multiple_tables.v3.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] allowing multiple PQclear() calls

2012-12-11 Thread Josh Kupershmidt
On Tue, Dec 11, 2012 at 5:18 AM, Boszormenyi Zoltan z...@cybertec.at wrote:
 2012-12-11 12:45 keltezéssel, Simon Riggs írta:

 On 11 December 2012 10:39, Marko Kreen mark...@gmail.com wrote:

 On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt schmi...@gmail.com
 wrote:

 Would it be crazy to add an already_freed flag to the pg_result
 struct which PQclear() would set, or some equivalent safety mechanism,
 to avoid this hassle for users?

 Such mechanism already exist - you just need to set
 your PGresult pointer to NULL after each PQclear().

 So why doesn't PQclear() do that?


 Because then PQclear() would need a ** not a *. Do you want its
 interface changed for 9.3 and break compatibility with previous versions?
 Same can be said for e.g. PQfinish(). Calling it again crashes your client,
 as I have recently discovered when I added atexit() functions that
 does if (conn) PQfinish(conn);  and the normal flow didn't do conn = NULL;
 after it was done.

Ah, well. I guess using a macro like:

#define SafeClear(res) do {PQclear(res); res = NULL;} while (0);

will suffice for me.

Josh


-- 
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 errors from 9.2.1 and 9.2.2 (I hope I'm missing something obvious)

2012-12-11 Thread Josh Kupershmidt
On Tue, Dec 11, 2012 at 6:01 PM, David Gould da...@sonic.net wrote:

 I'm sure I've had a stroke or something in the middle of the night and just
 didn't notice, but I'm able to reproduce the following on three different
 hosts on both 9.2.1 and 9.2.2. As far as I know the only difference between
 these queries is whitespace since I just up-arrowed them in psql and
 deleted a space or lf. And as far as I can tell none of these errors are
 correct.

 Complete transcript, freshly started 9.2.2.

 dg@jekyl:~$ psql
 psql (9.2.2)
 Type help for help.

 dg=# CREATE TABLE t (
  i INTEGER,
  PRIMARY KEY (i)
 );
 ERROR:  type key does not exist
 LINE 3:  PRIMARY KEY (i)

Hrm, although I didn't see such characters in your above text, perhaps
you have some odd Unicode characters in your input. For example, the
attached superficially similar input file will generate the same error
message for me. (The odd character in my input is U+2060, 'Word
Joiner', encoded 0xE2 0x81 0xA0.)

Josh


test.sql
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] Multiple --table options for other commands

2012-12-11 Thread Josh Kupershmidt
On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc k...@meme.com wrote:
 Hi Josh,

 I've signed up to review this patch.

Thanks!

 I configured with assertions, built, and tested using
 the attached script.  It seems to do what it's supposed
 to and the code looks ok to me.

 The docs build.  The text is reasonable.

 I also diffed the output of the attached script with
 the output of an unpatched head and got what I expected.

Cool test script.

 Yes, the current pg_restore silently
 ignores multiple --table arguments, and seems to use the last
 one.  You are introducing a backwards incompatible
 change here.  I don't know what to do about it, other
 than perhaps to have the patch go into 10.0 (!?) and
 introduce a patch now that complains about multiple
 --table arguments.  On the other hand, perhaps it's
 simply undocumented what pg_restore does when
 given repeated, conflicting, arguments and we're
 free to change this.  Any thoughts?

Agreed with Robert that this change should be reasonable in a major
version (i.e. 9.3).

 On 12/10/2012 09:23:03 PM, Karl O. Pinc wrote:
 On 10/30/2012 10:14:19 PM, Josh Kupershmidt wrote:

  I went ahead and cooked up a patch to allow pg_restore, clusterdb,
  vacuumdb, and reindexdb to accept multiple --table switches. Hope I
  didn't overlook a similar tool, but these were the only other
  commands
  I found taking a --table argument.

 I believe you need ellipses behind --table in the syntax summaries
 of the command reference docs.

Hrm, I was following pg_dump's lead here for the .sgml docs, and
didn't see anywhere that pg_dump makes the multiple --table syntax
explicit other than in the explanatory text underneath the option.

 I also note that the pg_dump --help output says table(s) so
 you probably want to have pg_restore say the same now that it
 takes multiple tables.

Good catch, will fix, and ditto reindexdb's --index help output. (It
is possible that the --help output for pg_dump was worded to say
table(s) because one can use a pattern --table specification with
pg_dump, though IMO it's helpful to mention table(s) in the --help
output for the rest of these programs as well, as a little reminder to
the user.)

Josh


multiple_tables.v2.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


[HACKERS] allowing multiple PQclear() calls

2012-12-10 Thread Josh Kupershmidt
The documentation for PQclear() doesn't say whether it is safe to call
PQclear() more than once on the same PGresult pointer. In fact, it is
not safe, but apparently only because of this last step:
/* Free the PGresult structure itself */
free(res);

The other members of PGresult which may be freed by PQclear are set to
NULL or otherwise handled so as not to not be affected by a subsequent
PQclear().

I find that accounting for whether I've already PQclear'ed a given
PGresult can be quite tedious in some cases. For example, in the
cleanup code at the end of a function where control may goto in case
of a problem, it would be much simpler to unconditionally call
PQclear() without worrying about whether this was already done. One
can see an admittedly small illustration of this headache in
pqSetenvPoll() in our own codebase, where several times PQclear(res);
is called immediately before a goto error_return;

Would it be crazy to add an already_freed flag to the pg_result
struct which PQclear() would set, or some equivalent safety mechanism,
to avoid this hassle for users?

Josh


-- 
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] Suggestion for --truncate-tables to pg_restore

2012-12-04 Thread Josh Kupershmidt
Sorry for the delay in following up here.

On Mon, Nov 26, 2012 at 8:30 PM, Karl O. Pinc k...@meme.com wrote:
 On 11/26/2012 08:45:08 PM, Josh Kupershmidt wrote:
 On Mon, Nov 26, 2012 at 3:42 PM, Robert Haas robertmh...@gmail.com
 wrote:
  On Mon, Nov 26, 2012 at 4:51 PM, Karl O. Pinc k...@meme.com wrote:
  P.S.  An outstanding question regards --truncate-tables
  is whether it should drop indexes before truncate
  and re-create them after restore.  Sounds like it should.
 
  Well, that would improve performance, but it also makes the
 behavior
  of object significantly different from what one might expect from
 the
  name.  One of the problems here is that there seem to be a number
 of
  slightly-different things that one might want to do, and it's not
  exactly clear what all of them are, or whether a reasonable number
 of
  options can cater to all of them.

 Another problem: attempting to drop a unique constraint or primary
 key
 (if we're counting these as indexes to be dropped and recreated,
 which
 they should be if the goal is reasonable restore performance) which
 is
 referenced by another table's foreign key will cause:
   ERROR:  cannot drop constraint xxx on table yyy
   because other objects depend on it

 and as discussed upthread, it would be impolite for pg_restore to
 presume it should monkey with dropping+recreating other tables'
 constraints to work around this problem, not to mention impossible
 when pg_restore is not connected to the target database.

 I'm thinking impossible because it's impossible to know
 what the existing FKs are without a db connection.  Impossible is
 a problem.  You may have another reason why it's impossible.

Yes, that's what I meant.

 Meanwhile it sounds like the --truncate-tables patch
 is looking less and less desirable.  I'm ready for
 rejection, but will soldier on in the interest of
 not wasting other people work on this, if given
 direction to move forward.

Well, as far as I was able to tell, the use-case where this patch
worked without trouble was limited to restoring a table, or schema
with table(s), that:
 a.) has some view(s) dependent on it
 b.) has no other tables with FK references to it, so that we don't run into:
ERROR:  cannot truncate a table referenced in a foreign key constraint
 c.) is not so large that it takes forever for data to be restored
with indexes and constraints left intact
 d.) and whose admin does not want to use --clean plus a list-file
which limits pg_restore to the table and its views

I was initially hoping that the patch would be more useful for
restoring a table with FKs pointing to it, but it seems the only
reliable way to do this kind of selective restore with pg_restore is
with --clean and editing the list-file. Editing the list-file is
certainly tedious and prone to manual error, but I'm not sure this
particular patch has a wide enough use-case to alleviate that pain
significantly.

Josh


-- 
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] Suggestion for --truncate-tables to pg_restore

2012-11-26 Thread Josh Kupershmidt
On Mon, Nov 26, 2012 at 3:42 PM, Robert Haas robertmh...@gmail.com wrote:
 On Mon, Nov 26, 2012 at 4:51 PM, Karl O. Pinc k...@meme.com wrote:
 P.S.  An outstanding question regards --truncate-tables
 is whether it should drop indexes before truncate
 and re-create them after restore.  Sounds like it should.

 Well, that would improve performance, but it also makes the behavior
 of object significantly different from what one might expect from the
 name.  One of the problems here is that there seem to be a number of
 slightly-different things that one might want to do, and it's not
 exactly clear what all of them are, or whether a reasonable number of
 options can cater to all of them.

Another problem: attempting to drop a unique constraint or primary key
(if we're counting these as indexes to be dropped and recreated, which
they should be if the goal is reasonable restore performance) which is
referenced by another table's foreign key will cause:
  ERROR:  cannot drop constraint xxx on table yyy
  because other objects depend on it

and as discussed upthread, it would be impolite for pg_restore to
presume it should monkey with dropping+recreating other tables'
constraints to work around this problem, not to mention impossible
when pg_restore is not connected to the target database.

It is a common administrative task to selectively restore some
existing tables' contents from a backup, and IIRC was the impetus for
this patch. Instead of adding a bunch of options to pg_restore,
perhaps a separate tool specific to this task would be the way to go.
It could handle the minutiae of truncating, dropping and recreating
constraints and indexes of the target tables, and dealing with FKs
sensibly, without worrying about conflicts with existing pg_restore
options and behavior.

Josh


-- 
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] Suggestion for --truncate-tables to pg_restore

2012-11-23 Thread Josh Kupershmidt
On Wed, Nov 21, 2012 at 5:48 AM, Karl O. Pinc k...@meme.com wrote:

 On Fri, Sep 21, 2012 at 8:54 AM, Karl O. Pinc k...@meme.com wrote:
  On 09/20/2012 12:24:49 PM, Karl O. Pinc wrote:

 OT:
 After looking at the code I found a number of  conflicting
 option combinations are not tested for or rejected.   I can't
 recall what they are now.  The right way to fix these would be
 to send in a separate patch for each conflict all attached
 to one email/commitfest item?  Or one patch that just gets
 adjusted until it's right?

Typically I think it's easiest for the reviewer+committer to consider
a bunch of such similar changes altogether in one patch, rather than
in a handful of separate patches, though that's just my own
preference.

  
 
  More serious objections were raised regarding semantics.
 
  What if, instead, the initial structure looked like:
 
  CREATE TABLE schemaA.foo
(id PRIMARY KEY, data INT);
 
  CREATE TABLE schemaB.bar
(id INT CONSTRAINT bar_on_foo REFERENCES foo
   , moredata INT);
 
  With a case like this, in most real-world situations, you'd
  have to use pg_restore with --disable-triggers if you wanted
  to use --data-only and --truncate-tables.  The possibility of
  foreign key referential integrity corruption is obvious.

 Why would --disable-triggers be used in this example? I don't think
 you could use --truncate-tables to restore only table foo, because
 you would get:

   ERROR:  cannot truncate a table referenced in a foreign key
 constraint

 (At least, I think so, not having tried with the actual patch.) You
 could use --disable-triggers when restoring bar.

 I tried it and you're quite right.  (I thought I'd tried this
 before, but clearly I did not -- proving the utility of the review
 process.  :-)  My assumption was that since triggers
 were turned off that constraints, being triggers, would be off as
 well and therefore tables with foreign keys could be truncated.
 Obviously not, since the I get the above error.

 I just tried it.  --disable-triggers does not disable constraints.

Just to be clear, I believe the problem in this example is that
--disable-triggers does not disable any foreign key constraints of
other tables when you are restoring a single table. So with:

  pg_restore -1 --truncate-tables --disable-triggers --table=foo \
  --data-only my.dump ...

you would get the complaint

  ERROR:  cannot truncate a table referenced in a foreign key constraint

which is talking about bar's referencing foo, and there was no

  ALTER TABLE bar DISABLE TRIGGER ALL

done, since bar isn't being restored. I don't have a quibble with
this existing behavior -- you are instructing pg_restore to only mess
with bar, after all. See below, though, for a comparison of --clean
and --truncate-tables when restoring foo and bar together.

 For your first example, --truncate-tables seems to have some use, so
 that the admin isn't forced to recreate various views which may be
 dependent on the table. (Though it's not too difficult to work around
 this case today.)

 As an aside: I never have an issue with this, having planned ahead.
 I'm curious what the not-too-difficult work-around is that you have
 in mind.  I'm not coming up with a tidy way to, e.g, pull a lot
 of views out of a dump.

Well, for the first example, there was one table and only one view
which depended on the table, so manually editing the list file like
so:

  pg_restore --list --file=my.dump  output.list
  # manually edit file output.list, select only entries pertaining
  # to the table and dependent view
  pg_restore -1 --clean --use-list=output.list ...

isn't too arduous, though it would become so if you had more dependent
views to worry about.

I'm willing to count this use-case as a usability win for
--truncate-tables, since with that option the restore can be boiled
down to a short and sweet:

  pg_restore -1 --data-only --truncate-tables --table=mytable my.dump ...

Though note this may not prove practical for large tables, since
--data-only leaves constraints and indexes intact during the restore,
and can be massively slower compared to adding the constraints only
after the data has been COPYed in, as pg_restore otherwise does.

 For the second example involving FKs, I'm a little bit more hesitant
 about  endorsing the use of --truncate-tables combined with
 --disable-triggers to potentially allow bogus FKs. I know this is
 possible to some extent today using the --disable-triggers option,
 but
 it makes me nervous to be adding a mode to pg_restore if it's
 primarily designed to work together with --disable-triggers as a
 larger foot-gun.

 This is the crux of the issue, and where I was thinking of
 taking this patch.  I very often am of the mindset that
 foreign keys are no more or less special than other
 more complex data integrity rules enforced with triggers.
 (I suppose others might say the same regards to integrity
 enforced at the application level.)  This naturally
 inclines me to think that 

Re: [HACKERS] Suggestion for --truncate-tables to pg_restore

2012-11-20 Thread Josh Kupershmidt
Hi Karl,
I signed on to review this patch for the current CF. Most of the
background for the patch seems to be in the message below, so I'm
going to respond to this one first.

On Fri, Sep 21, 2012 at 8:54 AM, Karl O. Pinc k...@meme.com wrote:
 On 09/20/2012 12:24:49 PM, Karl O. Pinc wrote:

 I've had problems using pg_restore --data-only when
 restoring individual schemas (which contain data which
 has had bad things done to it).  --clean does not work
 well because of dependent objects in other schemas.

OK.

 

 First, the problem:

 Begin with the following structure:

 CREATE TABLE schemaA.foo (id PRIMARY KEY, data INT);

 CREATE VIEW schemaB.bar AS SELECT * FROM schemaA.foo;

 Then, by accident, somebody does:

 UPDATE schemaA.foo SET data = data + (RANDOM() * 1000)::INT;

 So, you want to restore the data into schemaA.foo.
 But schemaA.foo has (bad) data in it that must first
 be removed. It would seem that using

   pg_restore --clean -n schemaA -t foo my_pg_dump_backup

 would solve the problem, it would drop schemaA.foo,
 recreate it, and then restore the data.  But this does
 not work.  schemaA.foo does not drop because it's
 got a dependent database object, schemaB.bar.

Right.

 Of course there are manual work-arounds.  One of these
 is truncating schemaA.foo and then doing a pg_restore
 with --data-only.

Simply doing TRUNCATE manually was the first thought that occurred to
me when I saw your example.

 The manual work-arounds become
 increasingly burdensome as you need to restore more
 tables.  The case that motivated me was an attempt
 to restore the data in an entire schema, one which
 contained a significant number of tables.

TBH, I didn't find the example above particularly compelling for
demonstrating the need for this feature. If you've just got one table
with dependent views which needs to be restored, it's pretty easy to
manually TRUNCATE and have pg_restore --data-only reload the table.
(And easy enough to combine the truncate and restore into a single
transaction in case anything goes wrong, if need be.)

But I'm willing to grant that this proposed feature is potentially as
useful as existing restore-jiggering options like --disable-triggers.
And I guess I could see that if you're really stuck having to perform
a --data-only restore of many tables, this feature could come in
handy.

 So, the idea here is to be able to do a data-only
 restore, first truncating the data in the tables
 being restored to remove the existing corrupted data.

 The proposal is to add a --truncate-tables option
 to pg_restore.

 

 There were some comments on syntax.

 I proposed to use -u as a short option.  This was
 thought confusing, given it's use in other
 Unix command line programs (mysql).   Since there's
 no obvious short option, forget it.  Just have
 a long option.

Agreed.

 Another choice is to avoid introducing yet another
 option and instead overload --clean so that when
 doing a --data-only restore --clean truncates tables
 and otherwise --clean retains the existing behavior of
 dropping and re-creating the restored objects.

I like the --truncate-tables flag idea better than overloading
--clean, since it makes the purpose immediately apparent.

 (I tested pg_restore with 9.1 and when --data-only is
 used --clean is ignored, it does not even produce a warning.
 This is arguably a bug.)

+1 for having pg_restore bail out with an error if the user specifies
--data-only --clean. By the same token, --clean and --truncate-tables
together should also raise a not allowed error.

 

 More serious objections were raised regarding semantics.

 What if, instead, the initial structure looked like:

 CREATE TABLE schemaA.foo
   (id PRIMARY KEY, data INT);

 CREATE TABLE schemaB.bar
   (id INT CONSTRAINT bar_on_foo REFERENCES foo
  , moredata INT);

 With a case like this, in most real-world situations, you'd
 have to use pg_restore with --disable-triggers if you wanted
 to use --data-only and --truncate-tables.  The possibility of
 foreign key referential integrity corruption is obvious.

Why would --disable-triggers be used in this example? I don't think
you could use --truncate-tables to restore only table foo, because
you would get:

  ERROR:  cannot truncate a table referenced in a foreign key constraint

(At least, I think so, not having tried with the actual patch.) You
could use --disable-triggers when restoring bar. Though if you're
just enabling that option for performance purposes, and are unable to
guarantee that the restore will leave the tables in a consistent
state, well then it seems like you shouldn't use the option.

 Aside:  Unless you're restoring databases in their entirety
 the pg_restore --disable-triggers option makes it easy to
 introduce foreign key referential integrity corruption.

Yup, and I think the docs could do more to warn users about
--disable-triggers in particular. And I see you've submitted a
separate patch along those lines.

 In fact, since pg_restore 

Re: [HACKERS] Multiple --table options for other commands

2012-10-30 Thread Josh Kupershmidt
On Sun, Oct 28, 2012 at 4:30 PM, Josh Kupershmidt schmi...@gmail.com wrote:

 I see there's already a TODO for allowing pg_restore to accept
 multiple --table arguments[1], but would folks support adding this
 capability to various other commands which currently accept only a
 single --table argument, such as clusterdb, vacuumdb, and reindexdb?

I went ahead and cooked up a patch to allow pg_restore, clusterdb,
vacuumdb, and reindexdb to accept multiple --table switches. Hope I
didn't overlook a similar tool, but these were the only other commands
I found taking a --table argument.

If you run, say:
  pg_dump -Fc --table=tab1 --table=tab2 ... |
  pg_restore --table=tab1 --table=tab2 ...

without the patch, only tab2 will be restored and pg_restore will
make no mention of this omission to the user. With the patch, both
tables should be restored.

I also added support for multiple --index options for reindexdb, since
that use-case seemed symmetrical to --table. As suggested previously,
I moved the SimpleStringList types and functions out of pg_dump.h and
common.c into dumputils.{h,c}, so that the code in ./src/bin/scripts/
could use it.

If there are no objections, I can add the patch to the next CF.

Josh


multiple_tables.v1.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


[HACKERS] Multiple --table options for other commands

2012-10-28 Thread Josh Kupershmidt
Hi all,

I see there's already a TODO for allowing pg_restore to accept
multiple --table arguments[1], but would folks support adding this
capability to various other commands which currently accept only a
single --table argument, such as clusterdb, vacuumdb, and reindexdb?

Looking at pg_dump, it seems like these other commands would want to
use the SimpleStringList / SimpleStringCell machinery which currently
lives only in pg_dump. I'm guessing that ./src/bin/pg_dump/dumputils.c
would be the right place for such common code?

Josh

[1] http://archives.postgresql.org/pgsql-hackers/2010-08/msg00689.php


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


[HACKERS] string escaping in tutorial/syscat.source

2012-10-14 Thread Josh Kupershmidt
Hi all,
It seems the queries in ./src/tutorial/syscat.source use string
escaping with the assumption that standard_conforming_strings is off,
and thus give wrong results with modern versions. A simple fix is
attached.
Josh


syscat.source_escaping.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] pg_reorg in core?

2012-09-20 Thread Josh Kupershmidt
On Thu, Sep 20, 2012 at 7:05 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Hi all,

 During the last PGCon, I heard that some community members would be
 interested in having pg_reorg directly in core.

I'm actually not crazy about this idea, at least not given the current
state of pg_reorg. Right now, there are a quite a few fixes and
features which remain to be merged in to cvs head, but at least we can
develop pg_reorg on a schedule independent of Postgres itself, i.e. we
can release new features more often than once a year. Perhaps when
pg_reorg is more stable, and the known bugs and missing features have
been ironed out, we could think about integrating into core.

Granted, a nice thing about integrating with core is we'd probably
have more of an early warning when reshuffling of PG breaks pg_reorg
(e.g. the recent splitting of the htup headers), but such changes have
been quick and easy to fix so far.

 Just to recall, pg_reorg is a functionality developped by NTT that allows to
 redistribute a table without taking locks on it.
 The technique it uses to reorganize the table is to create a temporary copy
 of the table to be redistributed with a CREATE TABLE AS
 whose definition changes if table is redistributed with a VACUUM FULL or
 CLUSTER.
 Then it follows this mechanism:
 - triggers are created to redirect all the DMLs that occur on the table to
 an intermediate log table.

N.B. CREATE TRIGGER takes an AccessExclusiveLock on the table, see below.

 - creation of indexes on the temporary table based on what the user wishes
 - Apply the logs registered during the index creation
 - Swap the names of freshly created table and old table
 - Drop the useless objects

 The code is hosted by pg_foundry here: http://pgfoundry.org/projects/reorg/.
 I am also maintaining a fork in github in sync with pgfoundry here:
 https://github.com/michaelpq/pg_reorg.

 Just, do you guys think it is worth adding a functionality like pg_reorg in
 core or not?

 If yes, well I think the code of pg_reorg is going to need some
 modifications to make it more compatible with contrib modules using only
 EXTENSION.
 For the time being pg_reorg is divided into 2 parts, binary and library.
 The library part is the SQL portion of pg_reorg, containing a set of C
 functions that are called by the binary part. This has been extended to
 support CREATE EXTENSION recently.
 The binary part creates a command pg_reorg in charge of calling the set of
 functions created by the lib part, being just a wrapper of the library part
 to control the creation and deletion of the objects.
 It is also in charge of deleting the temporary objects by callback if an
 error occurs.

 By using the binary command, it is possible to reorganize a single table or
 a database, in this case reorganizing a database launches only a loop on
 each table of this database.

 My idea is to remove the binary part and to rely only on the library part to
 make pg_reorg a single extension with only system functions like other
 contrib modules.

 In order to do that what is missing is a function that could be used as an
 entry point for table reorganization, a function of the type
 pg_reorg_table(tableoid) and pg_reorg_table(tableoid, text).
 All the functionalities of pg_reorg could be reproducible:
 - pg_reorg_table(tableoid) for a VACUUM FULL reorganization
 - pg_reorg_table(tableoid, NULL) for a CLUSTER reorganization if table has a
 CLUSTER key
 - pg_reorg_table(tableoid, columnname) for a CLUSTER reorganization based on
 a wanted column.

 Is it worth the shot?

I haven't seen this documented as such, but AFAICT the reason that
pg_reorg is split into a binary and set of backend functions which are
called by the binary is that pg_reorg needs to be able to control its
steps in several transactions so as to avoid holding locks
excessively. The reorg_one_table() function uses four or five
transactions per table, in fact. If all the logic currently in the
pg_reorg binary were moved into backend functions,  calling
pg_reorg_table() would have to be a single transaction, and there
would be no advantage to using such a function vs. CLUSTER or VACUUM
FULL.

Also, having a separate binary we should be able to perform some neat
tricks such as parallel index builds using multiple connections (I'm
messing around with this idea now). AFAIK this would also not be
possible if pg_reorg were contained solely in the library functions.

Josh


-- 
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_reorg in core?

2012-09-20 Thread Josh Kupershmidt
On Thu, Sep 20, 2012 at 8:33 PM, Michael Paquier
michael.paqu...@gmail.com wrote:

 On Fri, Sep 21, 2012 at 12:07 PM, Josh Kupershmidt schmi...@gmail.com
 wrote:

 On Thu, Sep 20, 2012 at 7:05 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:

 What could be also great is to move the project directly into github to
 facilitate its maintenance and development.

No argument from me there, especially as I have my own fork in github,
but that's up to the current maintainers.

 Granted, a nice thing about integrating with core is we'd probably
 have more of an early warning when reshuffling of PG breaks pg_reorg
 (e.g. the recent splitting of the htup headers), but such changes have
 been quick and easy to fix so far.

 Yes, that is also why I am proposing to integrate it into core. Its
 maintenance pace would be faster and easier than it is now in pgfoundry.

If the argument for moving pg_reorg into core is faster and easier
development, well I don't really buy that. Yes, there would presumably
be more eyeballs on the project, but you could make the same argument
about any auxiliary Postgres project which wants more attention, and
we can't have everything in core. And I fail to see how being in-core
makes development easier; I think everyone here would agree that the
bar to commit things to core is pretty darn high. If you're concerned
about the [lack of] development on pg_reorg, there are plenty of
things to fix without moving the project. I recently posted an issues
roundup to the reorg list, if you are interested in pitching in.

Josh


-- 
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] autocomplete - SELECT fx

2012-07-10 Thread Josh Kupershmidt
On Sat, Jul 7, 2012 at 5:43 PM, Noah Misch n...@leadboat.com wrote:
 I like the patch, as far as it goes.  It's the natural addition to the
 completions we already offer; compare the simplistic completion after WHERE.
 Like Pavel and Robert, I think a delightful implementation of tab completion
 for SELECT statements would require radical change.

Thanks for the feedback, Noah. Peter, are you interested in posting an
updated version of your patch? (The only problems I remember are
checking attisdropped and function visibility.)

Josh

-- 
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] psql \n shortcut for set search_path =

2012-07-10 Thread Josh Kupershmidt
On Tue, Jul 10, 2012 at 2:09 AM, Colin 't Hart co...@sharpheart.org wrote:
 Attached please find a trivial patch for psql which adds a \n meta command
 as a shortcut for typing set search_path =.

I think the use-case is a bit narrow: saving a few characters typing
on a command not everyone uses very often (I don't), at the expense of
adding yet another command to remember. Plus it opens the floodgates
to people wanting yet more separate commands for other possibly
commonly-used SET commands. There are a lot of GUCs, after all, even
counting only those changeable at runtime.

Josh

-- 
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] autocomplete - SELECT fx

2012-07-05 Thread Josh Kupershmidt
On Mon, Jul 2, 2012 at 1:13 AM, Pavel Stehule pavel.steh...@gmail.com wrote:

 I tested Peter's patch and it works well.

I liked it as well. But I'm not sure what should happen with the patch
now. It seems like it'd be commit-ready with just a tweak or two to
the query as I noted in my last mail, but Tom did seem opposed[1] to
the idea in the first thread, and no one else has spoken up recently
in favor of the idea, so maybe it should just be marked Rejected or
Returned with Feedback?

Josh

[1] http://archives.postgresql.org/message-id/13612.1328887227%40sss.pgh.pa.us

-- 
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] Posix Shared Mem patch

2012-07-03 Thread Josh Kupershmidt
On Tue, Jul 3, 2012 at 6:57 AM, Robert Haas robertmh...@gmail.com wrote:
 Here's a patch that attempts to begin the work of adjusting the
 documentation for this brave new world.  I am guessing that there may
 be other places in the documentation that also require updating, and
 this page probably needs more work, but it's a start.

I think the boilerplate warnings in config.sgml about needing to raise
the SysV parameters can go away; patch attached.

Josh


config.sgml.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] pg_signal_backend() asymmetry

2012-06-28 Thread Josh Kupershmidt
On Thu, Jun 28, 2012 at 6:48 AM, Noah Misch n...@leadboat.com wrote:
 On Thu, Jun 28, 2012 at 01:36:49AM -0700, Daniel Farina wrote:
 On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt schmi...@gmail.com wrote:
  I have one nitpick related to the recent changes for
  pg_cancel_backend() and pg_terminate_backend(). If you use these
  functions as an unprivileged user, and try to signal a nonexistent
  PID, you get:

 I think the goal there is to avoid leakage of the knowledge or
 non-knowledge of a given PID existing once it is deemed out of
 Postgres' control.  Although I don't have a specific attack vector in
 mind for when one knows a PID exists a-priori, it does seem like an
 unnecessary admission on the behalf of other programs.

 I think it was just an oversight.  I agree that these functions have no
 business helping users probe for live non-PostgreSQL PIDs on the server, but
 they don't do so and Josh's patch won't change that.  I recommend committing
 the patch.  Users will be able to probe for live PostgreSQL PIDs, but
 pg_stat_activity already provides those.

Yeah, I figured that since pg_stat_activity already shows users PIDs,
there should be no need to have pg_signal_backend() lie about whether
a PID was a valid backend or not. And if for some reason we did want
to lie, we should give an accurate message like not valid backend, or
must be superuser or have the same role

I noticed that I neglected to update the comment which was in the
non-superuser case: updated version attached.

On Thu, Jun 28, 2012 at 5:22 AM, Magnus Hagander mag...@hagander.net wrote:
 Well, there are two sides to it - one is the message, the other is if
 it should be ERROR or WARNING. My reading is that Josh is suggesting
 we make them WARNING in both cases, right?

Maybe I should have said I had a few nitpicks instead of just one
:-) A more detailed summary of the little issues I'm aiming to fix:

1a.) SIGNAL_BACKEND_NOPERMISSION being used for both invalid PID and
role mismatch, causing the must be superuser or have ... message to
be printed in both cases for non-superusers

1b.) Since SIGNAL_BACKEND_NOPERMISSION is used for both duties, you'll
get an ERROR instead of just a WARNING during plausibly-normal usage.
For example, if you're running
  SELECT pg_cancel_backend(pid)
  FROM pg_stat_activity
  WHERE usename = CURRENT_USER AND
  pid != pg_backend_pid();

you might be peeved if it ERROR'ed out with the bogus message claiming
the process was owned by someone else, when the backend has just
exited on its own. So even if we thought it was worth hiding to
non-superusers whether the PID is invalid or belongs to someone else,
I think the message should just be at WARNING level.

2a.) Existing code uses two calls to BackendPidGetProc() for
non-superusers in the error-free case.

2b.) I think the comment the check for whether it's a backend process
is below is a little misleading, since IsBackendPid() is just
checking whether the return of BackendPidGetProc() is NULL, which has
already been done.

3.) grammar fix for comment (on it's own - on its own)

Josh


pg_signal_backend_asymmetry.v2.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


[HACKERS] pg_signal_backend() asymmetry

2012-06-27 Thread Josh Kupershmidt
Hi all,

I have one nitpick related to the recent changes for
pg_cancel_backend() and pg_terminate_backend(). If you use these
functions as an unprivileged user, and try to signal a nonexistent
PID, you get:

  ERROR:  must be superuser or have the same role to cancel queries
running in other server processes

Whereas if you do the same thing as a superuser, you get:

  WARNING:  PID 123 is not a PostgreSQL server process
   pg_cancel_backend
  ---
   f
  (1 row)

The comment above the WARNING generated for the latter case says:

/*
 * This is just a warning so a loop-through-resultset
will not abort
 * if one backend terminated on its own during the run
 */

which is nice, but wouldn't unprivileged users want the same behavior?
Not to mention, the ERROR above is misleading, as it claims the
nonexistent PID really belongs to another user.

A simple fix is attached. The existing code called both
BackendPidGetProc(pid) and IsBackendPid(pid) while checking a
non-superuser's permissions, which really means two separate calls to
BackendPidGetProc(pid). I simplified that to down to a single
BackendPidGetProc(pid) call.

Josh


pg_signal_backend_asymmetry.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] return values of backend sub-main functions

2012-06-19 Thread Josh Kupershmidt
On Tue, Jun 19, 2012 at 4:31 AM, Peter Eisentraut pete...@gmx.net wrote:
 On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
 On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
   I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
   WalSenderMain(), and WalSndLoop() to return void as well.
 
  I agree this code is not very consistent or useful, but one question:
  what should the callers do if one of these functions *does* return?

 I was thinking of a two-pronged approach:  First, add
 __attribute__((noreturn)) to the functions.  This will cause a suitable
 compiler to verify on a source-code level that nothing actually returns
 from the function.  And second, at the call site, put an abort();  /*
 not reached */.  Together, this will make the code cleaner and more
 consistent, and will also help the compiler out a bit about the control
 flow.

 Patch for 9.3 attached.

+1. Should this call around line 4114 of postmaster.c not bother with
proc_exit() anymore:

/* And run the backend */
proc_exit(BackendRun(port));

I was hoping that some of the clang static analyzer complaints would
go away with these changes, though it looks like only one[1] did. I
would be interested to see the similar elog/ereport patch you
mentioned previously, perhaps that will eliminate a bunch of warnings.

Josh

[1] this one goes away with the patch:
http://kupershmidt.org/pg/scan-build-2012-06-19-master/report-E2cUqJ.html#EndPath

-- 
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] [BUGS] Tab completion of function arguments not working in all cases

2012-06-18 Thread Josh Kupershmidt
On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed dean.a.rash...@gmail.com wrote:
 On 18 June 2012 04:21, Josh Kupershmidt schmi...@gmail.com wrote:

 As a side note unrelated to this patch, I also dislike how function
 name tab-completions will not fill in the opening parenthesis, which
 makes for unnecessary work for the user, as one then has to type the
 parenthesis and hit tab again to get possible completions for the
 function arguments. The current behavior causes:
  DROP FUNCTION my_fTAB

 which completes to:
  DROP FUNCTION my_function

 enter parenthesis, and hit tab:
  DROP FUNCTION my_function(TAB

 which, if there is only one match, could complete to:
  DROP FUNCTION my_function(integer)

 when the last three steps could have been consolidated with better
 tab-completion. Perhaps this could be a TODO.


 Hmm, I find that it does automatically fill in the opening
 parenthesis, but only once there is a space after the completed
 function name. So
 DROP FUNCTION my_fTAB completes to DROP FUNCTION my_function 
 (note the space at the end). Then pressing TAB one more time gives
 DROP FUNCTION my_function ( , and then pressing TAB again gives
 the function arguments.

 Alternatively DROP FUNCTION my_functionTAB (no space after
 function name) first completes to DROP FUNCTION my_function  (adding
 the space), and then completes with the opening parenthesis, and then
 with the function arguments.

 It's a bit clunky, but I find that repeatedly pressing TAB is easier
 than typing the opening bracket.

Interesting, I see the behavior you describe on Linux, using psql +
libreadline, and the behavior I showed (i.e. repeatedly pressing tab
won't automatically fill in the function arguments after the function
name is completed, seemingly because no space is deposited after the
completed function name)  is with OS X 10.6, using psql + libedit.

[snip]
 you get tab-completions for both text) and bytea(, when you
 probably expected only the former. That's easy to fix though, please
 see attached v2 patch.

 Good catch.
 I think that's a useful additional test, and is also consistent with
 the existing code in Query_for_list_of_attributes.

OK, I'll mark Ready for Committer in the CF.

Josh

-- 
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: autocomplete for functions

2012-06-18 Thread Josh Kupershmidt
On Sun, Feb 19, 2012 at 12:10 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
 Hello

 I found so this extremely simple patch should be useful.

 It helps for pattern SELECT fx();

 There was thread about it.

Hi Pavel,
I signed up to be reviewer for this patch, and finally got around to
taking a look. This thread, and the thread about Peter's earlier patch
along the same lines have gotten a bit muddled, so allow me some recap
for my own sanity.

The first point to be addressed, is that Pavel's patch is basically a
subset of Peter's earlier[1] patch. Pavel's patch autocompletes

  SELECT TAB

with possible function names. Peter's patch autocompletes both
possible column names and possible function names. So, which version,
if any, would we want? Both Tom[2] and depesz[3] have asked for column
names to be autocompleted if we're going to go down this road, and I
personally would find completion of column names handy. Others [5][6]
have asked for function names to be (also?) autocompleted here, so it
seems reasonable that we'd want to include both.

I counted two general objections to Peter's patch in these threads, namely:

 1.) Complaints about the tab-completion not covering all cases,
possibly leading to user frustration at our inconsistency. [2] [4]
 2.) Concerns that the tab-completion wouldn't be useful given how
many results we'd have from system columns and functions [7]

I do agree these are two legitimate concerns. However, for #1, our
tab-completion is and has always been incomplete. I take the basic
goal of the tab-completion machinery to be offer a suggestion when
we're pretty sure we know what the user wants, otherwise stay quiet.
There are all sorts of places where our reliance on inspecting back
only a few words into the current line and not having a true command
parser prevents us from being able to make tab-completion guesses, but
that's been accepted so far, and I don't think it's fair to raise the
bar for this patch.

Re: concern #2, Tom complained about there being a bunch of possible
column and function completions in the regression test database. That
may be true, but if you look at this slightly-modified version of the
query Peter's patch proposes:

SELECT substring(name, 1, 3) AS sub, COUNT(*)
  FROM (
SELECT attname FROM pg_attribute WHERE NOT attisdropped
UNION
SELECT proname || '(' FROM pg_proc p WHERE
pg_catalog.pg_function_is_visible(p.oid)) t (name)
  GROUP BY sub ORDER BY COUNT(*) DESC;

I count only 384 distinct 3-length prefixes in an empty database,
thanks to many built-in columns and functions sharing the same prefix
(e.g. int or pg_). Obviously, there is plenty of room left for
3-length prefixes, out of the 27^3 or more possibilities. In some
casual mucking around in my own databases, I found the
column-completion rather useful, and typing 3 characters of a
column-name to be sufficient to give matches which were at least
non-builtin attributes, and often a single unique match.

There were some ideas down-thread about how we might filter out some
of the noise in these completions, which would be interesting. I'd be
happy with the patch as-is though, modulo the attisdropped and
pg_function_is_visible() tweaks to the query.

Josh


[1] 
http://archives.postgresql.org/message-id/1328820579.11241.4.camel%40vanquo.pezone.net
[2] http://archives.postgresql.org/message-id/7745.1328855069%40sss.pgh.pa.us
[3] http://www.depesz.com/2011/07/08/wish-list-for-psql/
[4] http://archives.postgresql.org/message-id/13612.1328887227%40sss.pgh.pa.us
[5] 
http://archives.postgresql.org/message-id/CA%2BTgmoY7wRGgBbFhKwfASqrNOPwZwCjfm1AhL82769Xx-SyduA%40mail.gmail.com
[6] 
http://archives.postgresql.org/message-id/20120210140637.GB19783%40ldn-qws-004.delacy.com
[7] http://archives.postgresql.org/message-id/9966.1331920074%40sss.pgh.pa.us

-- 
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: autocomplete for functions

2012-06-18 Thread Josh Kupershmidt
On Mon, Mar 19, 2012 at 1:01 PM, Alvaro Herrera
alvhe...@commandprompt.com wrote:

 I'm rather of the contrary opinion -- surely if we're going to complete
 function names, we should only complete those that are in schemas in the
 path; similarly for column names.

I think it makes sense to only include currently-visible functions,
but *not* only columns from currently visible tables, since we won't
know yet whether the user intends to schema-qualify the table name.

  (BTW I didn't check but does this
 completion work if I schema-qualify a column name?)

Peter's proposed tab-completion only kicks in for the column-name
itself. Keep in mind, the user might be trying to enter:
  SELECT  schema.table.column ...
  SELECT  table.column ...
  SELECT  table_alias.column ...
  SELECT  column ...

and presumably want to tab-complete the second token somehow. I'm a
bit leery about trying to tab-complete those first two, and the third
is right out. Just having the fourth would make me happy.

Josh

-- 
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] [BUGS] Tab completion of function arguments not working in all cases

2012-06-17 Thread Josh Kupershmidt
[Hope it's OK if I move this thread to -hackers, as part of CF review.]

On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed dean.a.rash...@gmail.com wrote:
 Hi,

 I noticed this while testing 9.2, but it seems to go back to at least
 8.3. Tab completion of function arguments doesn't work if the function
 is schema-qualified or double-quoted. So for example,

  DROP FUNCTION my_function ( TAB

 completes the functions arguments, but

  DROP FUNCTION my_schema.my_function ( TAB

 doesn't offer any completions, and nor does

  DROP FUNCTION my function ( TAB

+1 for the idea. I find the existing behavior rather confusing,
particularly the fact that a schema-qualified function name will be
tab-completed, i.e. this works.

  DROP FUNCTION my_schema.myTAB

but then, as your second example above shows, no completions are
subsequently offered for the function arguments.

As a side note unrelated to this patch, I also dislike how function
name tab-completions will not fill in the opening parenthesis, which
makes for unnecessary work for the user, as one then has to type the
parenthesis and hit tab again to get possible completions for the
function arguments. The current behavior causes:
  DROP FUNCTION my_fTAB

which completes to:
  DROP FUNCTION my_function

enter parenthesis, and hit tab:
  DROP FUNCTION my_function(TAB

which, if there is only one match, could complete to:
  DROP FUNCTION my_function(integer)

when the last three steps could have been consolidated with better
tab-completion. Perhaps this could be a TODO.

 The attached patch fixes these problems by introducing a new macro
 COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
 seems to be the nearest analogous code that covers all the edge cases.

Anyway, on to the review of the patch itself:

* Submission *

Patch applies cleanly to git head, and regression tests are not
expected for tab-completion enhancements.

* Features  Usability *

I've verified that tab-completing of the first argument to functions
for DROP FUNCTION and ALTER FUNCTION commands for the most part works
as expected. The one catch I noticed was that
Query_for_list_of_arguments wasn't restricting its results to
currently-visible functions, so with a default search_path, if you
have these two functions defined:

  public.doppelganger(text)
  my_schema.doppelganger(bytea)

and then try:

  DROP FUNCTION doppelganger(TAB

you get tab-completions for both text) and bytea(, when you
probably expected only the former. That's easy to fix though, please
see attached v2 patch.

* Coding *

The new macro COMPLETE_WITH_ARG seems fine. The existing code used
malloc() directly for DROP FUNCTION and ALTER FUNCTION
(tab-complete.c, around lines 867 and 2190), which AIUI is frowned
upon in favor of pg_malloc(). The patch avoids this ugliness by using
the new COMPLETE_WITH_ARG macro, so that's a nice fixup.

Overall, a nice fix for an overlooked piece of the tab-completion machinery.

Josh


tab-complete.funcargs.v2.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


[HACKERS] pg_restore logging inconsistency

2012-05-30 Thread Josh Kupershmidt
Hi all,

Bosco Rama recently complained[1] about not seeing a message printed
by pg_restore for each LO to be restored. The culprit seems to be the
different level passed to ahlog() for this status message:

pg_backup_archiver.c:   ahlog(AH, 2, restoring large object with OID %u\n, 
oid);
pg_backup_tar.c:ahlog(AH, 1, restoring large 
object OID %u\n, oid);

depending on whether one is restoring a tar-format or custom-format
dump. I think these messages should be logged at the same level, to
avoid this inconsistency. The attached patch logs them both with
level=1, and makes the message texts identical. Note, as of 9.0 there
is already a line like this printed for each LO:

pg_restore: executing BLOB 135004

so I could see the argument for instead wanting to hide the restoring
large object messages. However, the OP was interested in seeing
something like a status indicator for the lo_write() calls which may
take a long time, and the above message isn't really helpful for that
purpose as it is printed earlier in the restore process. Plus it seems
reasonable to make verbose mode, well, verbose.

Josh

[1] http://archives.postgresql.org/pgsql-general/2012-05/msg00456.php


pg_restore_lo_message.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] Draft release notes complete

2012-05-10 Thread Josh Kupershmidt
On Wed, May 9, 2012 at 8:11 PM, Bruce Momjian br...@momjian.us wrote:
 I have completed my draft of the 9.2 release notes, and committed it to
 git.  I am waiting for our development docs to build, but after 40
 minutes, I am still waiting:

This bit:
  Previously supplied years and year masks of less than four digits
wrapped inconsistently.

I first read as Previously-supplied years... instead of Previously,
years and year masks with

In line with what Robert said, IMO he should be credited on
pg_opfamily_is_visible(), particularly since it was his idea and code
originally IIRC. And a few more I'm familiar with with, such as psql's
\ir which he substantially revised, not to mention his
much-appreciated assistance with all the psql comment-displaying under
'E.1.3.9.2.1. Comments'.

Josh

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


[HACKERS] psql: server version check for \dO

2012-05-09 Thread Josh Kupershmidt
Hi all,

I think psql's \dO command is missing the server version check which
similar commands such as \dx use. Right now \dO errors out with:

test=# \dO
ERROR:  relation pg_catalog.pg_collation does not exist

when talking to servers older than 9.1, which don't have pg_collation.
Simple patch for listCollations() attached.

Josh
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index 8dfb570..2cfacd3
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*** listCollations(const char *pattern, bool
*** 3061,3066 
--- 3061,3073 
  	printQueryOpt myopt = pset.popt;
  	static const bool translate_columns[] = {false, false, false, false, false};
  
+ 	if (pset.sversion  90100)
+ 	{
+ 		fprintf(stderr, _(The server (version %d.%d) does not support collations.\n),
+ pset.sversion / 1, (pset.sversion / 100) % 100);
+ 		return true;
+ 	}
+ 
  	initPQExpBuffer(buf);
  
  	printfPQExpBuffer(buf,

-- 
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] Last gasp

2012-04-11 Thread Josh Kupershmidt
On Wed, Apr 11, 2012 at 8:59 AM, Peter Geoghegan pe...@2ndquadrant.com wrote:
 On 11 April 2012 15:35, Magnus Hagander mag...@hagander.net wrote:

 For example, Thom (and others) could collect a number of typo fixes in
 their own repo and then just ask for a merge.The advantage over just
 staging multiple commits and then submitting a patch would be that
 multiple people could work on it...

 This is hardly a radical idea at all - it's basically how git was
 really intended to be used at scale. Of course, unless some committer
 is going to make it their responsibility to merge those commits say
 every 3 months, there's no point in bothering. This could consolidate
 the number of typo commits to boot, as they could be rebased. TBH, I
 find it slightly embarrassing to have to ask a committer to fix a
 minor typo, and it's hardly reasonable to expect me to save my typos
 up.

 Big +1 from me.

Particularly for the docs, it'd be nice to have more committer
bandwidth available, if there's a reasonable way to do so without
causing needless merge work for existing committers. Like Peter, I
hate to bother busy committers with trivial typofixes, and sometimes I
just don't bother sending such changes in, and they get lost :-(

Maybe keeping doc/ as a 'git submodule' could work? Or, as Tom
suggests, adding a global committer who could focus on docs changes
would effectively solve the problem as well.

Josh

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


Re: [HACKERS] psql: tab completions for 'WITH'

2012-04-10 Thread Josh Kupershmidt
On Tue, Apr 10, 2012 at 10:38 AM, Peter Eisentraut pete...@gmx.net wrote:
 On tis, 2012-04-03 at 22:34 -0700, Josh Kupershmidt wrote:
 I noticed psql's tab-completion for 'WITH' is a bit overeager. If you
 try to tab-complete commands like:
   ALTER ROLE jsmith WITH [TAB]
   COPY tbl FROM 'filename' WITH [TAB]

 you'll get 'RECURSIVE' unhelpfully filled in. I think 'RECURSIVE'
 should only be suggested if 'WITH' is the first and only word of the
 line.

 Committed that.

Thanks!

 On a related note, I found it annoying that after fixing the above
 problem, trying:
     ALTER ROLE jsmith WITH [TAB]
     CREATE ROLE jsmith WITH [TAB]

 didn't suggest any tab-completions -- it only works if you leave off
 the 'WITH' noise word, which I happen to use.

 Hmm, but now you've set it up so that you can complete ALTER ROLE foo
 WITH WITH.  Were you aware of that?

D'oh, I overlooked that. Attached is v2: the diff is a tad lengthier
now, but that should fix it.

Josh


create_alter_role_tab_complete.v2.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


[HACKERS] psql: tab completions for 'WITH'

2012-04-03 Thread Josh Kupershmidt
Hi all,

I noticed psql's tab-completion for 'WITH' is a bit overeager. If you
try to tab-complete commands like:
  ALTER ROLE jsmith WITH [TAB]
  COPY tbl FROM 'filename' WITH [TAB]

you'll get 'RECURSIVE' unhelpfully filled in. I think 'RECURSIVE'
should only be suggested if 'WITH' is the first and only word of the
line.

On a related note, I found it annoying that after fixing the above
problem, trying:
ALTER ROLE jsmith WITH [TAB]
CREATE ROLE jsmith WITH [TAB]

didn't suggest any tab-completions -- it only works if you leave off
the 'WITH' noise word, which I happen to use.

Attached are fixes for both of these gripes. I'll add to the next CF.

Josh


overeager_with.diff
Description: Binary data


create_alter_role_tab_complete.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] vacuumlo issue

2012-03-20 Thread Josh Kupershmidt
On Tue, Mar 20, 2012 at 7:53 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 I'm not entirely convinced that that was a good idea.  However, so far
 as vacuumlo is concerned, the only reason this is a problem is that
 vacuumlo goes out of its way to do all the large-object deletions in a
 single transaction.  What's the point of that?  It'd be useful to batch
 them, probably, rather than commit each deletion individually.  But the
 objects being deleted are by assumption unreferenced, so I see no
 correctness argument why they should need to go away all at once.

I think you are asking for this option:

  -l LIMIT stop after removing LIMIT large objects

which was added in b69f2e36402aaa.

Josh

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


[HACKERS] misleading error message from connectMaintenanceDatabase()

2012-02-27 Thread Josh Kupershmidt
I noticed a misleading error message recently while using createdb. Try:

test=# CREATE ROLE dummy NOLOGIN;

Now, attempt to use createdb as that role. Here's 9.1.1:

$ createdb -Udummy testdb
createdb: could not connect to database postgres: FATAL:  role dummy
is not permitted to log in

And here is git head:

$ createdb -Udummy testdb
createdb: could not connect to databases postgres or template1
Please specify an alternative maintenance database.
Try createdb --help for more information.

Although I guess you could argue the latter message is technically
correct, since could not connect is true, the first error message
seems much more helpful. Plus, Please specify an alternative
maintenance database is a rather misleading hint to be giving in this
situation.

This seems to be happening because connectMaintenanceDatabase() is
passing fail_ok = true to connectDatabase(), which in turn just
returns NULL and doesn't print a PQerrorMessage() for the failed conn.
So connectMaintenanceDatabase() has no idea why the connection really
failed.

A simple fix would be just to pass fail_ok = false for the last
connectDatabase() call inside connectMaintenanceDatabase(), and give
up on trying to tack on a likely-misleading hint about the maintenance
database. Patch attached. This leads to:

$ createdb -Udummy testdb
createdb: could not connect to database template1: FATAL:  role
dummy is not permitted to log in

which is almost the same as the 9.1.1 output, with the exception that
template1 is mentioned by default instead of the postgres
database.

Josh


connectMDB_error.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] disable prompting by default in createuser

2012-02-05 Thread Josh Kupershmidt
On Wed, Feb 1, 2012 at 1:13 PM, Peter Eisentraut pete...@gmx.net wrote:
 On sön, 2012-01-15 at 18:14 -0500, Josh Kupershmidt wrote:
 I see this patch includes a small change to dropuser, to make the
 'username' argument mandatory if --interactive is not set, for
 symmetry with createuser's new behavior. That's dandy, though IMO we
 shouldn't have -i be shorthand for --interactive with dropuser,
 and something different with createuser (i.e. we should just get rid
 of the i alias for dropuser).

 Well, all the other tools also support -i for prompting.

Taking a look at the current ./src/bin/scripts executables, I see only
2 out of 9 (`dropdb` and `dropuser`) which have -i mean
--interactive, and `reindexdb` has another meaning for -i
entirely. So I'm not sure there's such a clear precedent for having
-i mean --interactive within our scripts, at least.

 I'd rather get
 rid of -i for --inherit, but I fear that will break things as well.  I'm
 not sure what to do.

I think breaking backwards compatibility probably won't fly (and
should probably be handled by another patch, anyway). I guess it's OK
to keep the patch's current behavior, given we are already
inconsistent about what -i means.

 i.e. createuser tries taking either $PGUSER or the current username as
 a default user to create, while dropuser just bails out. Personally, I
 prefer just bailing out if no create/drop user is specified, but
 either way I think they should be consistent.

 That is intentional long-standing behavior.  createdb/dropdb work the
 same way.

OK.

Josh

-- 
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] Dry-run mode for pg_archivecleanup

2012-02-01 Thread Josh Kupershmidt
On Tue, Jan 31, 2012 at 10:29 AM, Gabriele Bartolini
gabriele.bartol...@2ndquadrant.it wrote:

 Here is my final version which embeds comments from Josh. I have also added
 debug information to be printed in case '-d' is given.

Looks fine; will mark Ready For Committer.

Josh

-- 
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] Dry-run mode for pg_archivecleanup

2012-01-27 Thread Josh Kupershmidt
On Fri, Jan 27, 2012 at 9:47 AM, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Jan 15, 2012 at 5:05 PM, Josh Kupershmidt schmi...@gmail.com wrote:
 On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
 gabriele.bartol...@2ndquadrant.it wrote:

 My actual intention was to have the filename as output of the command, in
 order to easily pipe it to another script. Hence my first choice was to
 use the stdout channel, considering also that pg_archivecleanup in dry-run
 mode is harmless and does not touch the content of the directory.

 Oh, right - I should have re-read your initial email before diving
 into the patch. That all makes sense given your intended purpose. I
 guess your goal of constructing some simple way to pass the files
 which would be removed on to another script is a little different than
 what I initially thought the patch would be useful for, namely as a
 testing/debugging aid for an admin.

 Perhaps both goals could be met by making use of '--debug' together
 with '--dry-run'. If they are both on, then an additional message like
 pg_archivecleanup: would remove file ...  would be printed to
 stderr, along with just the filename printed to stdout you already
 have.

 This email thread seems to have trailed off without reaching a
 conclusion.  The patch is marked as Waiting on Author in the
 CommitFest application, but I'm not sure that's accurate.  Can we try
 to nail this down?

Perhaps my last email was a bit wordy. The only real change I am
suggesting for Gabriele's patch is that the message printed to stderr
when debug + dryrun are activated be changed to would remove file
... from removing file, i.e around line 124:

if (debug)
fprintf(stderr, %s: %s file \%s\\n,
progname, (dryrun ? would remove : removing),
WALFilePath);

Other than that little quibble, I thought the patch was fine.

Josh

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


Re: [HACKERS] Psql names completion.

2012-01-26 Thread Josh Kupershmidt
On Mon, Jan 23, 2012 at 5:28 PM, Dominik Bylica dominik2c...@gmail.com wrote:
 Hello.

 It's probably not the right place to write, but I guess you are there to
 take care of it.

 When I was creating a trigger with command like:
 create trigger asdf before update on beginninOfTheNameOfMyTable...
 I hit tab and I got:
 create trigger asdf before update on SET

 That was obviously not what I expected to get.

Should be fixed in 9.2.

Josh

-- 
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] Dry-run mode for pg_archivecleanup

2012-01-15 Thread Josh Kupershmidt
On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
gabriele.bartol...@2ndquadrant.it wrote:

 My actual intention was to have the filename as output of the command, in
 order to easily pipe it to another script. Hence my first choice was to
 use the stdout channel, considering also that pg_archivecleanup in dry-run
 mode is harmless and does not touch the content of the directory.

Oh, right - I should have re-read your initial email before diving
into the patch. That all makes sense given your intended purpose. I
guess your goal of constructing some simple way to pass the files
which would be removed on to another script is a little different than
what I initially thought the patch would be useful for, namely as a
testing/debugging aid for an admin.

Perhaps both goals could be met by making use of '--debug' together
with '--dry-run'. If they are both on, then an additional message like
pg_archivecleanup: would remove file ...  would be printed to
stderr, along with just the filename printed to stdout you already
have.

Josh

-- 
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] disable prompting by default in createuser

2012-01-15 Thread Josh Kupershmidt
On Thu, Dec 22, 2011 at 2:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 On lör, 2011-11-26 at 01:28 +0200, Peter Eisentraut wrote:
 I propose that we change createuser so that it does not prompt for
 anything by default.  We can arrange options so that you can get prompts
 for whatever is missing, but by default, a call to createuser should
 just run CREATE USER with default options.  The fact that createuser
 issues prompts is always annoying when you create setup scripts, because
 you have to be careful to specify all the necessary options, and they
 are inconsistent and different between versions (although the last
 change about that was a long time ago), and the whole behavior seems
 contrary to the behavior of all other utilities.  I don't think there'd
 be a real loss by not prompting by default; after all, we don't really
 want to encourage users to create more superusers, do we?  Comments?

 Patch attached.  I'll add it to the next commitfest.

I looked at this patch for the 2012-01 CF. I like the idea, the
interactive-by-default behavior of createuser has always bugged me as
well.

I see this patch includes a small change to dropuser, to make the
'username' argument mandatory if --interactive is not set, for
symmetry with createuser's new behavior. That's dandy, though IMO we
shouldn't have -i be shorthand for --interactive with dropuser,
and something different with createuser (i.e. we should just get rid
of the i alias for dropuser).

Another little inconsistency I see with the behavior when no username
to create or drop is given:

$  createuser
createuser: creation of new role failed: ERROR:  role josh already exists
$ dropuser
dropuser: missing required argument role name
Try dropuser --help for more information.

i.e. createuser tries taking either $PGUSER or the current username as
a default user to create, while dropuser just bails out. Personally, I
prefer just bailing out if no create/drop user is specified, but
either way I think they should be consistent.

Oh, and I think the leading whitespace of this help message:

printf(_(  --interactive prompt for missing role name
and attributes rather\n

should be made the same as for other commands with no short-alias, e.g.

printf(_(  --replication role can initiate replication\n));

Other than those little complaints, everything looks good.

Josh

-- 
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] Dry-run mode for pg_archivecleanup

2012-01-14 Thread Josh Kupershmidt
On Sun, Dec 11, 2011 at 9:52 AM, Gabriele Bartolini
gabriele.bartol...@2ndquadrant.it wrote:
 Hi guys,

  I have added the '-n' option to pg_archivecleanup which performs a dry-run
 and outputs the names of the files to be removed to stdout (making possible
 to pass the list via pipe to another process).

  Please find attached the small patch. I submit it to the CommitFest.

Hi Gabriele,

I have signed on to review this patch for the 2012-01 CF. The patch
applies cleanly, includes the necessary documentation, and implements
a useful feature.

I think the actual debugging line:

+   fprintf(stdout, %s\n, WALFilePath);

might need to be tweaked. First, it's printing to stdout, and I think
pg_archivecleanup intentionally sends all its output to stderr, so
that it may show up in the postmaster log. (I expect the dry-run mode
would often be used to test out an archive_cleanup_command, instead of
only in stand-alone mode, where stdout would be fine.)

Also, I think the actual message should be something a little more
descriptive than just the WALFilePath. In debug mode,
pg_archivecleanup prints out a message like:

fprintf(stderr, %s: removing file \%s\\n,
progname, WALFilePath);

I think we'd want to print something similar, i.e. would remove file 

Oh, and I think the removing file...  debug message above should not
be printed in dryrun-mode, lest we confuse the admin.

Other than that, everything looks good.

Josh

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-15 Thread Josh Kupershmidt
On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith g...@2ndquadrant.com wrote:
 Same-user cancels, but not termination.  Only this, and nothing more.

+1 from me on this approach. I think enough people have clamored for
this simple approach which solves the common-case.

 There's one obvious and questionable design decision I made to highlight.
  Right now the only consumers of pg_signal_backend are the cancel and
 terminate calls.  What I did was make pg_signal_backend more permissive,
 adding the idea that role equivalence = allowed, and therefore granting that
 to anything else that might call it.  And then I put a stricter check on
 termination.  This results in a redundant check of superuser on the
 termination check, and the potential for mis-using pg_signal_backend. . .

I think that's OK, it should be easy enough to reorganize if more
callers to pg_signal_backend() happen to come along.

The only niggling concern I have about this patch (and, I think, all
similar ones proposed) is the possible race condition between the
permissions checking and the actual call of kill() inside
pg_signal_backend(). I fooled around with this by using gdb to attach
to Backend #1, stuck a breakpoint right before the call to:

if (kill(-pid, sig))

and ran a pg_cancel_backend() of a same-role Backend #2. Then, with
the permissions checking passed, I exited Backend #2 and used a script
to call fork() in a loop until my system PIDs had wrapped around to a
few PIDs before the one Backend #2 had held. Then, I opened a few
superuser connections until I had one with the same PID which Backend
#2 previously had.

After I released the breakpoint in gdb on Backend #1, it happily sent
a SIGINT to my superuser backend.

I notice that BackendPidGetProc() has the comment:

  ...  Note that it is up to the caller to be
  sure that the question remains meaningful for long enough for the
  answer to be used ...

So someone has mulled this over before. It took my script maybe 15-20
minutes to cause a wraparound by running fork() in a loop, and that
wraparound would somehow have to occur within the short execution of
pg_signal_backend(). I'm not super worried about the patch from a
security perspective, just thought I'd point this out.

Josh

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


Re: [HACKERS] psql setenv command

2011-11-28 Thread Josh Kupershmidt
On Sat, Nov 26, 2011 at 11:02 AM, Andrew Dunstan and...@dunslane.net wrote:
 Also, should the malloc() of newval just use pg_malloc() instead?

 Yes, also done.

This bit is unnecessary, since pg_malloc() takes care of the error handling:

+   if (!newval)
+   {
+   psql_error(out of memory\n);
+   exit(EXIT_FAILURE);
+   }


Also, the help output for setenv bleeds over an 80-character terminal,
and it seems the rest of the help outputs try to stay under this
limit. And an OCD nitpick: most of the psql-ref.sgml examples show
'testdb' at the prompt, how about we follow along.

 Other than those small gripes, the patch looks fine. Attached is an
updated version to save you some keystrokes.

Josh
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 01f57c4..f97929b 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** lo_import 152801
*** 2242,2247 
--- 2242,2265 
  
  
varlistentry
+ termliteral\setenv [ replaceable class=parametername/replaceable [ replaceable class=parametervalue/replaceable ] ]/literal/term
+ 
+ listitem
+ para
+ Sets the environment variable replaceable
+ class=parametername/replaceable to replaceable
+ class=parametervalue/replaceable, or if the
+ replaceable class=parametervalue/replaceable is
+ not supplied, unsets the environment variable. Example:
+ programlisting
+ testdb=gt; userinput\setenv PAGER less/userinput
+ testdb=gt; userinput\setenv LESS -imx4F/userinput
+ /programlisting
+ /para
+ /listitem
+   /varlistentry
+ 
+   varlistentry
  termliteral\sf[+] replaceable class=parameterfunction_description/ /literal/term
  
  listitem
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9cc73be..e4b4eaa 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 1110,1115 
--- 1110,1160 
  		free(opt0);
  	}
  
+ 
+ 	/* \setenv -- set environment command */
+ 	else if (strcmp(cmd, setenv) == 0)
+ 	{
+ 		char	   *envvar = psql_scan_slash_option(scan_state,
+   OT_NORMAL, NULL, false);
+ 		char	   *envval = psql_scan_slash_option(scan_state,
+   OT_NORMAL, NULL, false);
+ 
+ 		if (!envvar)
+ 		{
+ 			psql_error(\\%s: missing required argument\n, cmd);
+ 			success = false;
+ 		}
+ 		else if (strchr(envvar,'=') != NULL)
+ 		{
+ 			psql_error(\\%s: environment variable name must not contain '='\n,
+ 	   cmd);
+ 			success = false;
+ 		}
+ 		else if (!envval)
+ 		{
+ 			/* No argument - unset the environment variable */
+ 			unsetenv(envvar);
+ 			success = true;
+ 		}
+ 		else
+ 		{
+ 			/* Set variable to the value of the next argument */
+ 			int len = strlen(envvar) + strlen(envval) + 1;
+ 			char	   *newval = pg_malloc(len + 1);
+ 
+ 			snprintf(newval, len+1, %s=%s, envvar, envval);
+ 			putenv(newval);
+ 			success = true;
+ 			/*
+ 			 * Do not free newval here, it will screw up the environment
+ 			 * if you do. See putenv man page for details. That means we
+ 			 * leak a bit of memory here, but not enough to worry about.
+ 			 */
+ 		}
+ 		free(envvar);
+ 		free(envval);
+ 	}
+ 
  	/* \sf -- show a function's source code */
  	else if (strcmp(cmd, sf) == 0 || strcmp(cmd, sf+) == 0)
  	{
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 4649e94..13d8bf6 100644
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*** slashUsage(unsigned short int pager)
*** 158,164 
  {
  	FILE	   *output;
  
! 	output = PageOutput(93, pager);
  
  	/* if you add/remove a line here, change the row count above */
  
--- 158,164 
  {
  	FILE	   *output;
  
! 	output = PageOutput(94, pager);
  
  	/* if you add/remove a line here, change the row count above */
  
*** slashUsage(unsigned short int pager)
*** 257,262 
--- 257,263 
  
  	fprintf(output, _(Operating System\n));
  	fprintf(output, _(  \\cd [DIR]  change the current working directory\n));
+ 	fprintf(output, _(  \\setenv NAME [VALUE]   set or unset environment variable\n));
  	fprintf(output, _(  \\timing [on|off]   toggle timing of commands (currently %s)\n),
  			ON(pset.timing));
  	fprintf(output, _(  \\! [COMMAND]   execute command in shell or start interactive shell\n));

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


Re: [HACKERS] psql setenv command

2011-11-20 Thread Josh Kupershmidt
On Wed, Nov 2, 2011 at 5:36 PM, Andrew Dunstan and...@dunslane.net wrote:
 Updated patch is attached - adding to Nov commitfest.

Review of the v2 patch:

* Submission Review
Patch applies with some fuzz and builds without warnings. I noticed
some tab characters being used in psql-ref.sgml where they shouldn't
be.

* Usability Review
I sort of doubt I'll use the feature myself, but there was at least
one +1 for the feature idea already, so it seems useful enough. That
said, the stated motivation for the patch:

  First, it would be useful to be able to set pager options and possibly other
 settings, so my suggestion is for a \setenv command that could be put in a
 .psqlrc file, something like:

seems like it would be more suited to being set in the user's
~/.profile (perhaps via an 'alias' if you didn't want the changes to
apply globally). So unless I miss something, the real niche for the
patch seems to be for people to interactively change environment
settings while within psql. Again, not something I'm keen on for my
own use, but if others think it's useful, that's good enough for me.

* Coding Review / Nitpicks
The patch implements \setenv via calls to unsetenv() and putenv(). As
the comment notes,

| That means we
| leak a bit of memory here, but not enough to worry about.

which seems true; the man page basically says there's nothing to be
done about the situation.

The calls to putenv() and unsetenv() are done without any real input
checking. So this admittedly-pathological case produces surprising
results:

test=# \setenv foo=bar baz
test=# \! echo $foo
bar=baz

Perhaps 'envvar' arguments with a '=' in the argument should just be
disallowed.

Also, should the malloc() of newval just use pg_malloc() instead?

* Reviewer Review
I haven't tested on Windows.

Josh

-- 
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: psql + libedit command history truncation (was: psql history vs. dearmor (pgcrypto))

2011-11-17 Thread Josh Kupershmidt
On Mon, Nov 14, 2011 at 7:04 PM, Josh Kupershmidt schmi...@gmail.com wrote:
 But it reminded me of another issue. With OS X 10.6.8, and otool -L
 reporting that psql depends on libedit version 2.11.0, the up-arrow
 recall of Tomas' query gets truncated around here:
  5I0/NTm+fFkB0McY9E2fAA [rest of the line missing]

For the curious, this does appear to be a hardcoded limit in libedit.
A bit of digging through a tarball of libedit-20110802-3.0 turned up
this line in el.h:

#define EL_BUFSIZ   ((size_t)1024)  /* Maximum line size*/

which you can bump up as a work-around.

Josh

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


[HACKERS] psql \ir filename normalization

2011-11-15 Thread Josh Kupershmidt
Hi all,

Commit c7f23494c1103f87bcf1ef7cbfcd626e73edb337 editorialized a bit on
Gurjeet Singh's patch to implement \ir for psql, particularly in
process_file(). Unfortunately, it looks like it broke the common case
of loading a .SQL file in psql's working directory. Consider the
following test case:

mkdir -p /tmp/psql_test/subdir/
mkdir -p /tmp/psql_test/path2/

echo SELECT 'hello 1';  /tmp/psql_test/hello.sql
echo SELECT 'hello from parent';  /tmp/psql_test/hello_parent.sql
echo SELECT 'hello from absolute path'; 
/tmp/psql_test/path2/absolute_path.sql
echo -e SELECT 'hello 2';\n\ir ../hello_parent.sql\n\ir
/tmp/psql_test/path2/absolute_path.sql 
/tmp/psql_test/subdir/hello2.sql
echo -e \ir hello.sql\n\ir subdir/hello2.sql  /tmp/psql_test/load.sql


If you try to load in load.sql from any working directory other than
/tmp/psql_test/ , you should correctly see four output statements.
However, if you:
  cd /tmp/psql_test/  psql test -f load.sql

You will get:

psql:load.sql:1: /hello.sql: No such file or directory
psql:load.sql:2: /subdir/hello2.sql: No such file or directory

Attached is a patch which fixes this, by recycling the bit of
Gurjeet's code which used last_slash. (I have a feeling there's a
simpler way to fix it which avoids the last_slash complications.)

Josh


psql_fix_ir.v2.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] proposal: psql concise mode

2011-11-14 Thread Josh Kupershmidt
On Mon, Nov 14, 2011 at 5:16 PM, Ross Reedstrom reeds...@rice.edu wrote:
 Concise output might look like (bikeshed argument: splat indicates
 columns squashed out):

  test=# \d+ foo
                          Table public.foo
  Column |  Type   # Storage #
  +-+-+
  a      | integer # plain   #
  b      | integer # plain   #
  Has OIDs: no

 or:

  Column |  Type   || Storage |
  +-++-+
  a      | integer || plain   |
  b      | integer || plain   |

 or even:

  Column |  Type   || Storage ||
  +-++-++
  a      | integer || plain   ||
  b      | integer || plain   ||

Yeah, that's an idea. And/or the table footer could list the omitted columns.

Josh

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


[HACKERS] psql + libedit command history truncation (was: psql history vs. dearmor (pgcrypto))

2011-11-14 Thread Josh Kupershmidt
On Mon, Nov 14, 2011 at 1:01 PM, Robert Haas robertmh...@gmail.com wrote:
 It looks like the problem is that the original has a blank line after
 the line that says Version: GnuPG v2.0.17 (GNU/Linux), but when you
 recall it from the query buffer, that extra blank line gets elided.

 The attached patch fixes it for me.  I'm a little worried it might
 cause a problem in some case I'm not thinking about, but I can't think
 of any such case right this minute.

(FYI, the patch does seem to fix the problem Tomas was complaining about.)

But it reminded me of another issue. With OS X 10.6.8, and otool -L
reporting that psql depends on libedit version 2.11.0, the up-arrow
recall of Tomas' query gets truncated around here:
  5I0/NTm+fFkB0McY9E2fAA [rest of the line missing]

i.e. it's keeping roughly 1021 characters. I was about to just chalk
that up to some limit in libedit's readline() implementation, but I
can see in my ~/.psql_history file that the entire query is logged.
Plus \e recalls the full query correctly. And if I up-arrow to recall
the query, then do anything to modify that recalled query (such as
typing a few characters at the end, then moving back or forth through
the history), then subsequent recalls of the query work fine.

So I'm not sure if this is a bug purely in libedit, or if there's
something amiss in psql. I saw a possibly-related complaint about
psql+libedit on Debian[1]. Anyone have a better guess about what's
going on?
Josh

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=603922

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


[HACKERS] fix for psql's \dd version check

2011-11-11 Thread Josh Kupershmidt
Someone (me) didn't get the version check for part of psql's \dd
command quite right. I was using a 9.2 client on a 9.1 server and got
this when I ran \dd:

ERROR:  function pg_catalog.pg_opfamily_is_visible(oid) does not exist
LINE 33:   AND pg_catalog.pg_opfamily_is_visible(opf.oid)

since pg_opfamily_is_visible() is only available for 9.2 and up.
Attached is a simple fix.

Josh


dd_opfamily_version_check.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] proposal: psql concise mode

2011-11-10 Thread Josh Kupershmidt
On Thu, Nov 10, 2011 at 3:41 PM, Bruce Momjian br...@momjian.us wrote:
 Have you tried \d+ with this psql mode:

        \pset format wrapped

 It wraps the data so it fits on the screen --- it is my default in my
 .psqlrc.

I think that's one of the many psql features I haven't experimented
with, thanks for the suggestion. It looks OK for some things, but I
find the column-wrapping behavior can be rather illegible, e.g.

create table test (
  some_column_name serial PRIMARY KEY,
  another_column_name integer NOT NULL,
  another_col integer, username text
);

tmp=# \d+ test
  Table public.test
 Column |  Type   |  Modifiers   | Storage | Stats target | Description
+-+--+-+--+-
 some_column_na.| integer | not null def.| plain   |  |
.me | |.ault nextval.| |  |
| |.('test_some_.| |  |
| |.column_name_.| |  |
| |.seq'::regcla.| |  |
| |.ss)  | |  |
 another_column.| integer | not null | plain   |  |
._name  | |  | |  |
 another_col| integer |  | plain   |  |
 username   | text|  | extende.|  |
| |  |.d   |  |



That wrapping is pretty ugly, and the culprit is all the wasted
horizontal space for Stats Target and Description in this case
(and probably for many users, who never set either column modifier).
That output might be much nicer if, instead of Modifiers, Column,
and Storage getting squeezed, the empty Stats Target and
Description column headers got squeezed instead, giving the
populated columns more horizontal space.

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


Re: [HACKERS] proposal: psql concise mode

2011-11-10 Thread Josh Kupershmidt
On Thu, Nov 10, 2011 at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 As I suggested, many more unexpected failures (e.g. \dnS+) pop up when
 talking to a 7.3 server. It's not a big deal, but it'd be nice if we
 could instead error out with a sorry, we're too lazy to try to
 support 7.3 on the meta-commands which fail thusly, and make the
 various else clauses more explicit about just how far back their
 support really goes.

 Probably not worth the trouble ... how many pre-7.4 servers are still in
 the wild, and of those, how many might somebody try to talk to with a
 modern psql?

 The more realistic direction of future change, I think, is that we move
 up the cutoff version so we can take out some code, rather than add
 more.  At the moment I'd find it a hard sell to drop support for 8.1 or
 later; so maybe there's not enough removable code to make it worth any
 effort.  But in a few more years it'd be worth doing.

I am 100% on board with dropping support for such old servers whenever
feasible, so as to cut down on the cruft in psql -- that's the only
reason I cared to go poking at this at all. I would suggest we bump
the minimum supported server version for psql up to 8.0 at some point
in the not-too-distant future, perhaps even for 9.2.

 What *would* be worth doing today, IMO, is ripping out pg_dump's support
 for servers older than 7.3 or 7.4; in particular getting rid of its
 kluges for server versions without pg_depend info.

Yeah, that was another can of worms I had in the back of my mind. I
think there's a good case for maintaining longer backwards
compatibility in pg_dump vs. psql, to help people upgrade an ancient
server to a modern one. But certainly, anything older than 7.3 or 7.4
is pushing the boundaries in terms of being supported.

Jsoh

-- 
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: psql concise mode

2011-11-09 Thread Josh Kupershmidt
[Sorry for not CC'ing the list before, I'm still getting used to the
new Gmail interface]

On Tue, Nov 8, 2011 at 11:05 PM, Josh Kupershmidt schmi...@gmail.com wrote:
 On Tue, Nov 8, 2011 at 10:04 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Josh Kupershmidt schmi...@gmail.com writes:
 We're essentially pretending that we support all server versions with
 this code, instead of erroring out on some definite old version and
 admitting sorry, can't do it. ...
 I think we should draw a line somewhere about just how far back psql
 must support,

 Says right at the top of the file:

  * Support for the various \d (describe) commands.  Note that the current
  * expectation is that all functions in this file will succeed when working
  * with servers of versions 7.4 and up.  It's okay to omit irrelevant
  * information for an old server, but not to fail outright.

 Oh, heh, I did miss that note. 7.4 is a reasonable target, I guess.
 It's not clear to me from that comment whether it's acceptable for the
 code to fail outright on servers older than 7.4, as in the snippet I
 posted, but I'm pretty sure that is what would happen. (I don't have a
 7.x install handy to test that theory, as I haven't been able to build
 anything older than 8.0.)

FWIW, I just played around with 7.4 and 7.3 servers. (I had some bad
memories of the older tarballs not building, but that must have been
only on OS X -- I can build at least back to 7.3 on this Ubuntu 11.04
machine.)

Most meta-commands worked alright on 7.4, or at least failed
gracefully. The ones I saw which failed unexpectedly were \sf and \ef,
which complained:
  ERROR:  function pg_catalog.pg_get_functiondef(integer) does not exist

I think we need a server version check for these two meta-commands,
unless someone cares to make them work on  8.4, trivial patch
attached.

As I suggested, many more unexpected failures (e.g. \dnS+) pop up when
talking to a 7.3 server. It's not a big deal, but it'd be nice if we
could instead error out with a sorry, we're too lazy to try to
support 7.3 on the meta-commands which fail thusly, and make the
various else clauses more explicit about just how far back their
support really goes.

Josh
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 2c38902..121612c 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 583,589 
  	{
  		int			lineno = -1;
  
! 		if (!query_buf)
  		{
  			psql_error(no query buffer\n);
  			status = PSQL_CMD_ERROR;
--- 583,595 
  	{
  		int			lineno = -1;
  
! 		if (pset.sversion  80400)
! 		{
! psql_error(The server (version %d.%d) does not support editing function source.\n,
! 		   pset.sversion / 1, (pset.sversion / 100) % 100);
! status = PSQL_CMD_ERROR;
! 		}
! 		else if (!query_buf)
  		{
  			psql_error(no query buffer\n);
  			status = PSQL_CMD_ERROR;
*** exec_command(const char *cmd,
*** 1115,1121 
  		func_buf = createPQExpBuffer();
  		func = psql_scan_slash_option(scan_state,
  	  OT_WHOLE_LINE, NULL, true);
! 		if (!func)
  		{
  			psql_error(function name is required\n);
  			status = PSQL_CMD_ERROR;
--- 1121,1133 
  		func_buf = createPQExpBuffer();
  		func = psql_scan_slash_option(scan_state,
  	  OT_WHOLE_LINE, NULL, true);
! 		if (pset.sversion  80400)
! 		{
! psql_error(The server (version %d.%d) does not support showing function source.\n,
! 		   pset.sversion / 1, (pset.sversion / 100) % 100);
! status = PSQL_CMD_ERROR;
! 		}
! 		else if (!func)
  		{
  			psql_error(function name is required\n);
  			status = PSQL_CMD_ERROR;

-- 
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: psql concise mode

2011-11-08 Thread Josh Kupershmidt
On Mon, Nov 7, 2011 at 11:25 PM, Robert Haas robertmh...@gmail.com wrote:
 But I can't help feeling that as we continue to add more features,
 we've eventually going to end up with our backs to the wall.  Not sure
 what to do about that, but...

Seriously, parts of psql are starting to become a real mess.

[tangentially related rant]

I cringe whenever I have to go digging around in describe.c.
describeOneTableDetails() is what, 1100+ lines?! Doubtless, some
refactoring of that function would help. But the
backwards-compatibility burden isn't helping the situation. The first
conditional block based on pset.sversion in that function contains:

...
else if (pset.sversion = 8)
{
[some query against pg_catalog.pg_class]
}
else
{
printfPQExpBuffer(buf,
  SELECT relchecks, relkind, relhasindex, relhasrules, 
  reltriggers  0, relhasoids, 
  '', ''\n
  FROM pg_catalog.pg_class WHERE oid = '%s';,
  oid);
}

We're essentially pretending that we support all server versions with
this code, instead of erroring out on some definite old version and
admitting sorry, can't do it. The latter query would really break on
a 7.1 [*] or earlier server (thanks to relhasoids). Other pieces of
the same function would fail on 7.2 or earlier, e.g. due to querying
pg_depend or pg_namespace. Other code will fail on 7.3 or earlier,
e.g. due to querying pg_user.

I think we should draw a line somewhere about just how far back psql
must support, and don't worry about having crufty maybe it works but
who knows exactly how far back code for further support than that. I
think 8.0 would be a very generous backwards compatibility target.

Josh

--
[*] I based these claims about how far back the code would actually
work based on perusing old catalog doc pages, like:
http://www.postgresql.org/docs/7.3/static/catalogs.html
It's possible some of the old doc pages are incorrect, but I think my
point still stands.

-- 
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: psql concise mode

2011-11-07 Thread Josh Kupershmidt
On Mon, Nov 7, 2011 at 10:04 PM, Robert Haas robertmh...@gmail.com wrote:
 I don't strongly object to this, but I wonder how useful it will
 really be in practice.  It strikes me as the sort of advanced psql
 hackery that only a few people will use, and only some of those will
 gain any benefit.

I'm probably just not going to bother, based on the (lack of) response
so far. I'd like to have it for myself, but not enough to make the
patch and then fight for its inclusion :-)

 Empty columns don't really take up that much screen
 width, and even one value in any given column will require its
 inclusion anyway.

What really prompted the proposal was my somewhat antiquated use of
80-column terminal windows (so that 2 or 3 fit side-by-side
comfortably on my screen). A lot of the backslash commands are
creeping well over that 80-column limit, even with the most basic of
outputs, and I find the default line-wrapping hard to follow. And I'd
bet that the use of column comments and column statistics targets, for
example, are quite rare -- and that's almost 30 columns of horizontal
space lost for the common case of \d+ tablename.

Maybe I just have to get comfortable with the expanded mode.

  I can also see myself turning it on and then going
 - oh, wait, is that column not there, or did it just disappear because
 I'm in concise mode?

Yeah, that would be a bit of a nuisance in some cases.

 Not saying we shouldn't do it, just some food for thought.

Thanks for the feedback, anyway.

Josh

-- 
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: psql concise mode

2011-11-06 Thread Josh Kupershmidt
On Sun, Nov 6, 2011 at 1:16 PM, Dickson S. Guedes lis...@guedesoft.net wrote:
 test=# \d+ foo
                         Table public.foo
  Column |  Type   | Storage
 +-+-
  a      | integer | plain
  b      | integer | plain
 Has OIDs: no

 Using your example, what if column 'b' has a comment and 'a' not? How
 the above output will be displayed?

Then the comments would be displayed as they previously were, like so:

 Table public.foo
 Column |  Type   | Storage | Description
+-+-+-
 a  | integer | plain   |
 b  | integer | plain   | some comment
Has OIDs: no

Josh

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


[HACKERS] proposal: psql concise mode

2011-11-05 Thread Josh Kupershmidt
Hi all,

The good news is that psql's backslash commands are becoming quite
thorough at displaying all information which could conceivably be of
interest about an object. The bad news is, psql's backslash commands
often produce a lot of noise and wasted output. (There was some
grumbling along these lines re: the recent Stats Target patch).

I'd like to propose a concise mode for psql, which users might turn
on via a \pset option. Concise mode would affect only the output of
psql's backslash commands. For output results which have some all-NULL
columns, as in:

test=# \d+ foo
 Table public.foo
 Column |  Type   | Modifiers | Storage | Stats target | Description
+-+---+-+--+-
 a  | integer |   | plain   |  |
 b  | integer |   | plain   |  |
Has OIDs: no

Concise mode would simply omit the all-NULL columns, so that the
output would look like this:

test=# \d+ foo
 Table public.foo
 Column |  Type   | Storage
+-+-
 a  | integer | plain
 b  | integer | plain
Has OIDs: no

For actually implementing this: it'd be nice if the changes could be
localized to printQuery(). Unfortunately, there are a few stragglers
such as describeOneTableDetails() which have their own notions about
how to print their output, so I'm not sure how straightforward/small
such a patch would be.

But I just wanted to throw the idea out there. Any thoughts?

Josh

-- 
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] Show statistics target in \d+

2011-11-04 Thread Josh Kupershmidt
On Fri, Nov 4, 2011 at 10:05 AM, Magnus Hagander mag...@hagander.net wrote:
 On Fri, Nov 4, 2011 at 14:53, Cédric Villemain
 Interesting, can the ouput be clear on the value being a default or an
 explicit stat target ? (not mandatory but I believe I would like to
 have it only when the stat target is jnot the default)

 It shows -1 when it's the default.

 We could map that to the string default if we want, or just NULL, I
 guess. Not sure which is best?

I thought -1 was just fine. The ALTER TABLE doc page is clear that -1
means the system default, and having another value may just confuse
users.

Josh

-- 
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] EXECUTE tab completion

2011-10-21 Thread Josh Kupershmidt
On Thu, Oct 20, 2011 at 5:16 PM, Andreas Karlsson andr...@proxel.se wrote:
 A new version is attached.

Looks fine. Marking ready for committer (CF 2011-11).

Josh

-- 
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] EXECUTE tab completion

2011-10-19 Thread Josh Kupershmidt
On Mon, Sep 26, 2011 at 5:03 PM, Andreas Karlsson andr...@proxel.se wrote:
 Magnus's patch for adding tab completion of views to the TABLE statement
 reminded me of a minor annoyance of mine -- that EXECUTE always completes
 with PROCEDURE as if it would have been part of CREATE TRIGGER ... EXECUTE
 even when it is the first word of the line.

+1

 Attached is a simple patch which adds completion of prepared statement names
 to the EXECUTE statement.

 What could perhaps be added is that if you press tab again after completing
 the prepared statement name you might want to see a single ( appear. Did
 not add that though since EXECUTE foo(); is not valid syntax (while
 EXECUTE foo(1); is) and I did not feel the extra lines of code to add a
 query to check if the number of expected parameters is greater than 0 were
 worth it.

Yeah, that doesn't seem worth the trouble. The patch looks fine to me;
it doesn't break the existing EXECUTE completion intended for CREATE
TRIGGER and seems to work OK on a few examples I tried.

I guess the only quibble I can see is that the two comment lines might
be better written together, to mimic the neighboring comment styles,
as in:
/* EXECUTE */
/* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
else if ...

Incidentally, I was wondering what the heck was up with a clause like this:
else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
 pg_strcasecmp(prev2_wd, EXECUTE) == 0)

though that looks to be some strange quirk of previous_word()'s behavior.

Josh

-- 
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] EXECUTE tab completion

2011-10-19 Thread Josh Kupershmidt
On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Josh Kupershmidt schmi...@gmail.com writes:
 Incidentally, I was wondering what the heck was up with a clause like this:
     else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
              pg_strcasecmp(prev2_wd, EXECUTE) == 0)

 Hmm, maybe || was meant not  ?  It seems pretty unlikely that the
 above test would ever trigger on valid SQL input.

Well, changing '' to '||' breaks the stated comment of the patch, namely:
  /* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */

I assume this is an accepted quirk of previous_word() since we have
this existing similar code:

/* DROP, but watch out for DROP embedded in other commands */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, DROP) == 0 
 pg_strcasecmp(prev2_wd, DROP) == 0)

and the patch does seem to auto-complete a beginning EXECUTE
correctly. We could probably use a comment somewhere explaining this
quirk.

Josh

-- 
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_comments (was: Allow \dd to show constraint comments)

2011-10-12 Thread Josh Kupershmidt
On Wed, Oct 12, 2011 at 2:49 PM, Robert Haas robertmh...@gmail.com wrote:
 So, I think the critical question for this patch is do we want
 this?.

Yep. Or put another way, are the gains worth having another system
view we'll have to maintain forever?

 Tom didn't like it,

In [1], Tom seemed to be mainly angling for fixing up psql instead,
which has been done now. I didn't see a specific reason against adding
the view, other than it cannot be changed without an initdb. That's
a valid concern of course, but it applies equally well to other system
views.

[snip]
 On the third hand, Josh's previous batch of changes to clean up
 psql's behavior in this area are clearly a huge improvement: you can
 now display the comment for nearly anything by running the appropriate
 \dfoo command for whatever the object type is.  So ... is this still
 a good idea, or should we just forget about it?

I think this question is a part of a broader concern, namely do we
want to create and support system views for easier access to
information which is already available in different ways through psql
commands, or by manually digging around in the catalogs? I believe
there are at least several examples of existing views we maintain
which are very similar to pg_comments: pg_seclabel seems quite
similar, for instance.

Josh

--
[1] http://archives.postgresql.org/pgsql-hackers/2010-09/msg01081.php

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


Re: [HACKERS] psql setenv command

2011-09-26 Thread Josh Kupershmidt
On Thu, Sep 15, 2011 at 7:02 PM, Andrew Dunstan and...@dunslane.net wrote:
 On Thu, September 15, 2011 6:10 pm, Josh Kupershmidt wrote:
 [need way to show current values]
 \! echo $foo

 (which is how I tested the patch, of course)

Ah, right. IMO it'd be helpful to mention that echo example in your
changes to psql-ref.sgml, either as part of your example inside the
programlisting, or as a suggestion with the rest of the text.

BTW, have you tested this on Windows? I don't have a Windows machine
handy to fool with, but I do see what might be a mess/confusion on
that platform. The MSDN docs claim[1] that putenv() is deprecated in
favor of _putenv(). The code in pg_upgrade uses
SetEnvironmentVariableA on WIN32, while win32env.c has a wrapper
function pgwin32_putenv() around _putenv().

On Sat, Sep 24, 2011 at 5:18 PM, Jeff Janes jeff.ja...@gmail.com wrote:
 A description of the \setenv command should show up in the output of \?.

Yeah, Andrew agreed upthread that help.c should be amended as well,
which would fix \?.

 Should there be a regression test for this?  I'm not sure how it would
 work, as I don't see a cross-platform way to see what the variable is
 set to.

Similar recent psql changes haven't had regression tests included, and
I don't see much of a need here either.

--
[1] http://msdn.microsoft.com/en-US/library/ms235321%28v=VS.80%29.aspx

Josh

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


  1   2   >