Re: [HACKERS] proposal: make NOTIFY list de-duplication optional
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
On Tue, May 27, 2014 at 10:01 PM, Gurjeet Singh 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
On Thu, May 29, 2014 at 4:06 PM, Andrew Dunstan wrote: > On 05/29/2014 02:38 PM, Tom Lane wrote: >> Josh Kupershmidt 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=msg&goto=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] Odd uuid-ossp behavior on smew and shearwater
On Wed, May 28, 2014 at 11:23 PM, Tom Lane 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=smew&dt=2014-05-28%2023%3A38%3A28
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=shearwater&dt=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
i
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt 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
On Thu, Oct 10, 2013 at 12:54 PM, Andrew Dunstan 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
On Tue, Aug 27, 2013 at 1:14 PM, Heikki Linnakangas 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
On Tue, Jul 2, 2013 at 7:47 AM, Pavel Stehule 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
[HACKERS] tab-completion for \lo_import
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
On Tue, Jul 2, 2013 at 5:39 AM, Pavel Stehule wrote: > 2013/3/8 Josh Kupershmidt : >> On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule >> wrote: >>> 2013/3/8 Josh Kupershmidt : >> >>>> 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] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Fri, Jul 5, 2013 at 12:16 PM, Pavel Stehule 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
Re: [HACKERS] vacuumlo - use a cursor
On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan 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
On Thu, Jun 27, 2013 at 11:47 AM, Fujii Masao wrote:
> On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt wrote:
>> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao 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
On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao wrote: > On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu 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
On Tue, Jun 25, 2013 at 2:28 AM, Hari Babu 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] pg_filedump 9.3: checksums (and a few other fixes)
On Tue, Jun 18, 2013 at 12:42 PM, Jeff Davis 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=15554&forum_id=44&group_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
Re: [HACKERS] [9.4 CF 1] The Commitfest Slacker List
On Mon, Jun 24, 2013 at 12:57 PM, Josh Berkus 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
[HACKERS] isolationtester and 'specs' subdirectory
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
On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule 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?
On Fri, Mar 22, 2013 at 9:35 PM, Joshua D. Drake 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
On Wed, Mar 20, 2013 at 2:00 AM, Simon Riggs wrote: > On 20 March 2013 02:51, Michael Paquier 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
On Mon, Mar 18, 2013 at 3:10 PM, Robins Tharakan 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
On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule wrote: > 2013/3/8 Josh Kupershmidt : >> 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
[Moving to -hackers]
On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule 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
On Mon, Mar 4, 2013 at 11:54 AM, Stephen Frost 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
On Mon, Mar 4, 2013 at 11:39 AM, Stephen Frost 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
On Wed, Feb 27, 2013 at 12:09 PM, Stephen Frost 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
On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule wrote: > 2013/1/14 Tom Lane : >> 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
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
On Tue, Jan 15, 2013 at 6:35 PM, Jeff Janes 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
On Mon, Jan 7, 2013 at 8:12 PM, Peter Eisentraut wrote: > On Sat, 2013-01-05 at 15:34 +0100, Magnus Hagander wrote: >> On Fri, Jan 4, 2013 at 3:36 AM, Josh Kupershmidt 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
On Sat, Jan 5, 2013 at 7:34 AM, Magnus Hagander wrote: > On Fri, Jan 4, 2013 at 3:36 AM, Josh Kupershmidt 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
The ./src/bin/pg_dump README contains several inoperable examples. First, this suggestion: | or to list tables: | | pg_restore --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 -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
On Thu, Dec 20, 2012 at 1:26 AM, Gavin Flower 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
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
On Sun, Dec 2, 2012 at 4:37 AM, Alastair Turner 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
On Thu, Dec 13, 2012 at 9:03 PM, Karl O. Pinc 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: > > > > >--table >-t > > table > > 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
On Thu, Dec 13, 2012 at 7:24 PM, Karl O. Pinc 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
On Thu, Dec 13, 2012 at 6:05 AM, Karl O. Pinc 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 wrote: >> > > On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote: >> > >> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc >> > 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 ... 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
On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc wrote: > On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote: >> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc 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 node instead of . I've changed that one to be like the other two, and made them all have: 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] Multiple --table options for other commands
On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc 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
Re: [HACKERS] Strange errors from 9.2.1 and 9.2.2 (I hope I'm missing something obvious)
On Tue, Dec 11, 2012 at 6:01 PM, David Gould 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 . 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] allowing multiple PQclear() calls
On Tue, Dec 11, 2012 at 5:18 AM, Boszormenyi Zoltan wrote:
> 2012-12-11 12:45 keltezéssel, Simon Riggs írta:
>
>> On 11 December 2012 10:39, Marko Kreen wrote:
>>>
>>> On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt
>>> 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
[HACKERS] allowing multiple PQclear() calls
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
Sorry for the delay in following up here. On Mon, Nov 26, 2012 at 8:30 PM, Karl O. Pinc wrote: > On 11/26/2012 08:45:08 PM, Josh Kupershmidt wrote: >> On Mon, Nov 26, 2012 at 3:42 PM, Robert Haas >> wrote: >> > On Mon, Nov 26, 2012 at 4:51 PM, Karl O. Pinc 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
On Mon, Nov 26, 2012 at 3:42 PM, Robert Haas wrote: > On Mon, Nov 26, 2012 at 4:51 PM, Karl O. Pinc 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
On Wed, Nov 21, 2012 at 5:48 AM, Karl O. Pinc wrote: >> On Fri, Sep 21, 2012 at 8:54 AM, Karl O. Pinc 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
Re: [HACKERS] Suggestion for --truncate-tables to pg_restore
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 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
Re: [HACKERS] Multiple --table options for other commands
On Sun, Oct 28, 2012 at 4:30 PM, Josh Kupershmidt 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
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
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?
On Thu, Sep 20, 2012 at 8:33 PM, Michael Paquier wrote: > On Fri, Sep 21, 2012 at 12:07 PM, Josh Kupershmidt > wrote: >> >> On Thu, Sep 20, 2012 at 7:05 PM, Michael Paquier >> 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] pg_reorg in core?
On Thu, Sep 20, 2012 at 7:05 PM, Michael Paquier 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] [PATCH] psql \n shortcut for set search_path =
On Tue, Jul 10, 2012 at 2:09 AM, Colin 't Hart 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
On Sat, Jul 7, 2012 at 5:43 PM, Noah Misch 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] autocomplete - SELECT fx
On Mon, Jul 2, 2012 at 1:13 AM, Pavel Stehule 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
On Tue, Jul 3, 2012 at 6:57 AM, Robert Haas 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
On Thu, Jun 28, 2012 at 6:48 AM, Noah Misch 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 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 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
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
On Tue, Jun 19, 2012 at 4:31 AM, Peter Eisentraut 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 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] patch: autocomplete for functions
On Mon, Mar 19, 2012 at 1:01 PM, Alvaro Herrera 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] patch: autocomplete for functions
On Sun, Feb 19, 2012 at 12:10 PM, Pavel Stehule 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
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] [BUGS] Tab completion of function arguments not working in all cases
On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed wrote:
> On 18 June 2012 04:21, Josh Kupershmidt 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_f
>>
>> which completes to:
>> DROP FUNCTION my_function
>>
>> enter parenthesis, and hit tab:
>> DROP FUNCTION my_function(
>>
>> 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_f" completes to "DROP FUNCTION my_function "
> (note the space at the end). Then pressing one more time gives
> "DROP FUNCTION my_function ( ", and then pressing again gives
> the function arguments.
>
> Alternatively "DROP FUNCTION my_function" (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 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] [BUGS] Tab completion of function arguments not working in all cases
[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 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 (
>
> completes the functions arguments, but
>
> DROP FUNCTION my_schema.my_function (
>
> doesn't offer any completions, and nor does
>
> DROP FUNCTION "my function" (
+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.my
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_f
which completes to:
DROP FUNCTION my_function
enter parenthesis, and hit tab:
DROP FUNCTION my_function(
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(
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
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
On Wed, May 9, 2012 at 8:11 PM, Bruce Momjian 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
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
On Wed, Apr 11, 2012 at 8:59 AM, Peter Geoghegan wrote: > On 11 April 2012 15:35, Magnus Hagander 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'
On Tue, Apr 10, 2012 at 10:38 AM, Peter Eisentraut 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'
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
On Tue, Mar 20, 2012 at 7:53 AM, Tom Lane 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()
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
On Wed, Feb 1, 2012 at 1:13 PM, Peter Eisentraut 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
On Tue, Jan 31, 2012 at 10:29 AM, Gabriele Bartolini 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
On Fri, Jan 27, 2012 at 9:47 AM, Robert Haas wrote: > On Sun, Jan 15, 2012 at 5:05 PM, Josh Kupershmidt wrote: >> On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini >> 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.
On Mon, Jan 23, 2012 at 5:28 PM, Dominik Bylica 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] disable prompting by default in createuser
On Thu, Dec 22, 2011 at 2:26 PM, Peter Eisentraut 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
On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini 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] Dry-run mode for pg_archivecleanup
On Sun, Dec 11, 2011 at 9:52 AM, Gabriele Bartolini 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
On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith 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
On Sat, Nov 26, 2011 at 11:02 AM, Andrew Dunstan 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
+ \setenv [ name [ value ] ]
+
+
+
+ Sets the environment variable name to value, or if the
+ value is
+ not supplied, unsets the environment variable. Example:
+
+ testdb=> \setenv PAGER less
+ testdb=> \setenv LESS -imx4F
+
+
+
+
+
+
\sf[+] function_description
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
On Wed, Nov 2, 2011 at 5:36 PM, Andrew Dunstan 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))
On Mon, Nov 14, 2011 at 7:04 PM, Josh Kupershmidt 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
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
[HACKERS] psql + libedit command history truncation (was: psql history vs. dearmor (pgcrypto))
On Mon, Nov 14, 2011 at 1:01 PM, Robert Haas 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
Re: [HACKERS] proposal: psql concise mode
On Mon, Nov 14, 2011 at 5:16 PM, Ross Reedstrom 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] fix for psql's \dd version check
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
On Thu, Nov 10, 2011 at 6:12 PM, Tom Lane 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
On Thu, Nov 10, 2011 at 3:41 PM, Bruce Momjian 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
[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 wrote:
> On Tue, Nov 8, 2011 at 10:04 PM, Tom Lane wrote:
>> Josh Kupershmidt 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
On Mon, Nov 7, 2011 at 11:25 PM, Robert Haas 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
On Mon, Nov 7, 2011 at 10:04 PM, Robert Haas 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
On Sun, Nov 6, 2011 at 1:16 PM, Dickson S. Guedes 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
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+
On Fri, Nov 4, 2011 at 10:05 AM, Magnus Hagander 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
On Thu, Oct 20, 2011 at 5:16 PM, Andreas Karlsson 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
On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane wrote: > Josh Kupershmidt 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] EXECUTE tab completion
On Mon, Sep 26, 2011 at 5:03 PM, Andreas Karlsson 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] pg_comments (was: Allow \dd to show constraint comments)
On Wed, Oct 12, 2011 at 2:49 PM, Robert Haas 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 > \d 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
On Thu, Sep 15, 2011 at 7:02 PM, Andrew Dunstan 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 , 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 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
