[HACKERS] [PATCH] Patch to fix a crash of psql
hi When i test psql under multi-lingual and different encoding environment, I found a crash of psql. -- $ export PGCLIENTENCODING=SJIS $ psql psql (9.2rc1) Type help for help. postgres=# \i sql CREATE DATABASE You are now connected to database mydb as user postgres. CREATE SCHEMA Segmentation fault (core dumped) $ -- I'm look into this problem and found that only some especial character can cause psql crash. conditions is: 1. some especial character (my sql file contains japanese comment -- コメント . It can cause psql crash.) 2. PGCLIENTENCODING is SJIS 3. the encoding of input sql file is UTF-8 I investigated this problem. The reasons are as follows. -- src/bin/psql/mainloop.c - psql_scan_setup()//Set up to perform lexing of the given input line. --prepare_buffer ()//Set up a flex input buffer to scan the given data. malloc character buffer. set two \0 characters. (Flex wants two \0 characters after the actual data.) working in an unsafe encoding, the copy has multibyte sequences replaced by FFs to avoid fooling the lexer rules. the encoding of input sql file is different from PGCLIENTENCODING, two \0 characters are replaced by FFs. yy_scan_buffer() //Setup the input buffer state to scan directly from a user-specified character buffer. because two \0 characters are replaced by FFs,yy_scan_buffer() return 0. input buffer state can not setup correctly. - psql_scan() //Do lexical analysis of SQL command text. -- yylex() //The main scanner function which does all the work. because input buffer state is not setup,so when access the input buffer state,segmentation fault is happened. -- I modify src/bin/psql/psqlscan.l to resolve this problem. The diff file refer to the attachment psqlscan.l.patch. Regards, Jiang Guiqing diff --git a/src/bin/psql/psqlscan.l b/src/bin/psql/psqlscan.l index d32a12c..6c14298 100644 --- a/src/bin/psql/psqlscan.l +++ b/src/bin/psql/psqlscan.l @@ -1807,7 +1807,7 @@ prepare_buffer(const char *txt, int len, char **txtcopy) /* first byte should always be okay... */ newtxt[i] = txt[i]; i++; - while (--thislen 0) + while (--thislen 0 i len) newtxt[i++] = (char) 0xFF; } } CREATE DATABASE mydb; \connect mydb CREATE SCHEMA myschema; -- 繧ｳ繝｡繝ｳ繝� CREATE TABLE myschema.weather ( cityvarchar(80), temp_lo int, temp_hi int, prcpreal, datedate); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: index support for regexp search
On Mon, November 26, 2012 20:49, Alexander Korotkov wrote:
trgm-regexp-0.6.patch.gz
I ran the simple-minded tests against generated data (similar to the ones I did
in January 2012).
The problems of that older version seem pretty much all removed. (although I
didn't do much work
on it -- just reran these tests).
I used two 2 instances, 'HEAD' and 'trgm_regex', which were both compiled with
'--enable-depend' '--with-openssl' '--with-perl' '--with-libxml'
Tables used:
rowcount size tablesize index (trgm)
azjunk4 10^4 rows 1,171,456 | 9,781,248
azjunk5 10^5 rows 11,706,368 | 65,093,632
azjunk6 10^6 rows117,030,912 | 726,310,912
azjunk7 10^7 rows 1,170,292,736 | 4,976,189,440
(See my previous emails for a generating script)
Tables contain random generated text:
table azjunk7 limit 5;
txt
--
i kzzhv ssaa zv x xlepzxsgbdkxev v wn dmvqkuwb qxkyvgab gpidaosaqbewqimmai
jxj
bvwn zbevtzyhibbn hoctxurutn pvlatjjyxf f runa owpltbcunrbq ux peoook
rxwoscbytz
bbjlbbhhkivjivklgbh tvapzogh rj ky ahvgkvvlfudotvqapznludohdoyqrp
kvothyclbckbxu
hvic gomewbp izsjifqggyqgzcghdat lb kud ltfqaxqxjjom qkw wqggikgvph yi
sftmbjt
edbjfl vtcasudjpgfgjaf swooxygsse flnqd pxzsdmesqhqbzgirswysote muakq agk p w
uq
(5 rows)
with index on column 'txt':
create index az7_idx on azjunk7 using gin (txt gin_trgm_ops);
Queries were of the form:
explain analyze select txt from azjunkXX where txt ~ '$REGEX';
The main problem with the January version was that it chose to use the trgm
index even when it
could take a long time (hours). This has been resolved as far as I can see,
and the results are
now very attractive.
(There does seem to be a very slight regression on the seqscan, but it's so
small that I'm not yet
sure it's not noise)
Hardware: AMD FX-8120 with Linux 2.6.32-279.14.1.el6.x86_64 x86_64 GNU/Linux
PostgreSQL
9.3devel-trgm_regex-20121127_2353-e78d288c895bd296e3cb1ca29c7fe2431eef3fcd on
x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.7.2, 64-bit
port instancetableregex rows method
expl.analyze timing
6543 HEADazjunk4 x[ae]q 46 Seq Scan
12.962 ms
6554 trgm_regex azjunk4 x[ae]q 46 Bitmap Heap Scan
0.800 ms
6543 HEADazjunk4 x[ae]{1}q 46 Seq Scan
12.487 ms
6554 trgm_regex azjunk4 x[ae]{1}q 46 Bitmap Heap Scan
0.209 ms
6543 HEADazjunk4 x[ae]{1,1}q 46 Seq Scan
12.266 ms
6554 trgm_regex azjunk4 x[ae]{1,1}q 46 Bitmap Heap Scan
0.210 ms
6543 HEADazjunk4 x[ae]{,2}q 0 Seq Scan
14.322 ms
6554 trgm_regex azjunk4 x[ae]{,2}q 0 Bitmap Heap Scan
0.610 ms
6543 HEADazjunk4 x[ae]{,10}q 0 Seq Scan
20.503 ms
6554 trgm_regex azjunk4 x[ae]{,10}q 0 Bitmap Heap Scan
0.511 ms
6543 HEADazjunk4 x[ae]{1,2}q 49 Seq Scan
13.060 ms
6554 trgm_regex azjunk4 x[ae]{1,2}q 49 Bitmap Heap Scan
0.429 ms
6543 HEADazjunk4 x[aei]q 81 Seq Scan
12.487 ms
6554 trgm_regex azjunk4 x[aei]q 81 Bitmap Heap Scan
0.367 ms
6543 HEADazjunk4 x[aei]{1}q 81 Seq Scan
12.132 ms
6554 trgm_regex azjunk4 x[aei]{1}q 81 Bitmap Heap Scan
0.336 ms
6543 HEADazjunk4 x[aei]{1,1}q81 Seq Scan
12.168 ms
6554 trgm_regex azjunk4 x[aei]{1,1}q81 Bitmap Heap Scan
0.319 ms
6543 HEADazjunk4 x[aei]{,2}q 0 Seq Scan
14.586 ms
6554 trgm_regex azjunk4 x[aei]{,2}q 0 Bitmap Heap Scan
0.621 ms
6543 HEADazjunk4 x[aei]{,10}q 0 Seq Scan
20.134 ms
6554 trgm_regex azjunk4 x[aei]{,10}q 0 Bitmap Heap Scan
0.552 ms
6543 HEADazjunk4 x[aei]{1,2}q89 Seq Scan
12.553 ms
6554 trgm_regex azjunk4 x[aei]{1,2}q89 Bitmap Heap Scan
0.916 ms
6543 HEADazjunk4 x[aei]{1,3}q89 Seq Scan
13.055 ms
6554 trgm_regex azjunk4 x[aei]{1,3}q89 Seq Scan
13.064 ms
6543 HEADazjunk4 x[aei]q 81 Seq Scan
11.856 ms
6554 trgm_regex azjunk4 x[aei]q 81 Bitmap Heap Scan
0.398 ms
6543 HEADazjunk4 x[aei]{1}q 81 Seq Scan
11.951 ms
6554 trgm_regex azjunk4 x[aei]{1}q 81 Bitmap Heap Scan
0.369 ms
6543 HEADazjunk4 x[aei]{1,1}q81 Seq Scan
12.750 ms
6554 trgm_regex azjunk4
[HACKERS] Refactoring standby mode logic
The code that reads the WAL from the archive, from pg_xlog, and from a
master server via walreceiver, is quite complicated. I'm talking about
the WaitForWALToBecomeAvailable() function in xlog.c. I got frustrated
with that while working on the switching timeline over streaming
replication patch.
Attached is a patch to refactor that logic into a more straightforward
state machine. It's always been a kind of a state machine, but it's been
hard to see, as the code wasn't explicitly written that way. Any objections?
The only user-visible effect is that this slightly changes the order
that recovery tries to read files from the archive, and pg_xlog, in the
presence of multiple timelines. At the moment, if recovery fails to find
a file on current timeline in the archive, it then tries to find it in
pg_xlog. If it's not found there either, it checks if the file on next
timeline exists in the archive, and then checks if exists in pg_xlog.
For example, if we're currently recovering timeline 2, and target
timeline is 4, and we're looking for WAL file A, the files are searched
for in this order:
1. File 0004000A in archive
2. File 0004000A in pg_xlog
3. File 0003000A in archive
4. File 0003000A in pg_xlog
5. File 0002000A in archive
6. File 0002000A in pg_xlog
With this patch, the order is:
1. File 0004000A in archive
2. File 0003000A in archive
3. File 0002000A in archive
4. File 0004000A in pg_xlog
5. File 0003000A in pg_xlog
6. File 0002000A in pg_xlog
This change should have no effect in normal restore scenarios. It'd only
make a difference if some files in the middle of the sequence of WAL
files are missing from the archive, but have been copied to pg_xlog
manually, and only if that file contains a timeline switch. Even then, I
think I like the new order better; it's easier to explain if nothing else.
- Heikki
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 512,523 static XLogwrtResult LogwrtResult = {0, 0};
/*
* Codes indicating where we got a WAL file from during recovery, or where
! * to attempt to get one. These are chosen so that they can be OR'd together
! * in a bitmask state variable.
*/
! #define XLOG_FROM_ARCHIVE (10) /* Restored using restore_command */
! #define XLOG_FROM_PG_XLOG (11) /* Existing file in pg_xlog */
! #define XLOG_FROM_STREAM (12) /* Streamed from master */
/*
* openLogFile is -1 or a kernel FD for an open log file segment.
--- 512,529
/*
* Codes indicating where we got a WAL file from during recovery, or where
! * to attempt to get one.
*/
! typedef enum
! {
! XLOG_FROM_ANY = 0, /* request to read WAL from any source */
! XLOG_FROM_ARCHIVE, /* restored using restore_command */
! XLOG_FROM_PG_XLOG, /* existing file in pg_xlog */
! XLOG_FROM_STREAM, /* streamed from master */
! } XLogSource;
!
! /* human-readable names for XLogSources, for debugging output */
! static const char *xlogSourceNames[] = { any, archive, pg_xlog, stream };
/*
* openLogFile is -1 or a kernel FD for an open log file segment.
***
*** 542,563 static XLogSegNo readSegNo = 0;
static uint32 readOff = 0;
static uint32 readLen = 0;
static bool readFileHeaderValidated = false;
! static int readSource = 0; /* XLOG_FROM_* code */
/*
! * Keeps track of which sources we've tried to read the current WAL
! * record from and failed.
*/
! static int failedSources = 0; /* OR of XLOG_FROM_* codes */
/*
* These variables track when we last obtained some WAL data to process,
* and where we got it from. (XLogReceiptSource is initially the same as
* readSource, but readSource gets reset to zero when we don't have data
! * to process right now.)
*/
static TimestampTz XLogReceiptTime = 0;
! static int XLogReceiptSource = 0; /* XLOG_FROM_* code */
/* Buffer for currently read page (XLOG_BLCKSZ bytes) */
static char *readBuf = NULL;
--- 548,575
static uint32 readOff = 0;
static uint32 readLen = 0;
static bool readFileHeaderValidated = false;
! static XLogSource readSource = 0; /* XLOG_FROM_* code */
/*
! * Keeps track of which source we're currently reading from. This is
! * different from readSource in that this is always set, even when we don't
! * currently have a WAL file open. If lastSourceFailed is set, our last
! * attempt to read from currentSource failed, and we should try another source
! * next.
*/
! static XLogSource currentSource = 0; /* XLOG_FROM_* code */
! static bool lastSourceFailed = false;
/*
* These variables track when we last obtained some WAL data to process,
* and where we got it from. (XLogReceiptSource is initially the same as
* readSource, but readSource gets reset to zero
Re: [HACKERS] [PATCH] Patch to fix a crash of psql
I confirmed the problem. Also I confirmed your patch fixes the problem. In addition to this, all the tests in test/mb and test/regress are passed. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp hi When i test psql under multi-lingual and different encoding environment, I found a crash of psql. -- $ export PGCLIENTENCODING=SJIS $ psql psql (9.2rc1) Type help for help. postgres=# \i sql CREATE DATABASE You are now connected to database mydb as user postgres. CREATE SCHEMA Segmentation fault (core dumped) $ -- I'm look into this problem and found that only some especial character can cause psql crash. conditions is: 1. some especial character (my sql file contains japanese comment -- コメント . It can cause psql crash.) 2. PGCLIENTENCODING is SJIS 3. the encoding of input sql file is UTF-8 I investigated this problem. The reasons are as follows. -- src/bin/psql/mainloop.c - psql_scan_setup() //Set up to perform lexing of the given input line. --prepare_buffer () //Set up a flex input buffer to scan the given data. malloc character buffer. set two \0 characters. (Flex wants two \0 characters after the actual data.) working in an unsafe encoding, the copy has multibyte sequences replaced by FFs to avoid fooling the lexer rules. the encoding of input sql file is different from PGCLIENTENCODING, two \0 characters are replaced by FFs. yy_scan_buffer() //Setup the input buffer state to scan directly from a user-specified character buffer. because two \0 characters are replaced by FFs,yy_scan_buffer() return 0. input buffer state can not setup correctly. - psql_scan() //Do lexical analysis of SQL command text. -- yylex() //The main scanner function which does all the work. because input buffer state is not setup,so when access the input buffer state,segmentation fault is happened. -- I modify src/bin/psql/psqlscan.l to resolve this problem. The diff file refer to the attachment psqlscan.l.patch. Regards, Jiang Guiqing -- Sent 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 in CREATE/DROP INDEX CONCURRENTLY
On Thursday, November 29, 2012 12:39 AM Tom Lane wrote. Andres Freund and...@2ndquadrant.com writes: On 2012-11-27 23:46:58 -0500, Tom Lane wrote: Attached is a very preliminary draft patch for this. I've not addressed the question of whether we can clear indcheckxmin during transactional updates of pg_index rows, but I think it covers everything else talked about in this thread. Attached is an updated patch for HEAD that I think is about ready to go. I'll start making a back-patchable version shortly. I had verified in the Patch committed that the problem is resolved. I have a doubt related to RelationGetIndexList() function. In while loop, if index is not live then it continues, so it can be possible that we don't find a valid index after this while loop. But still after the loop, it marks relation-rd_indexvalid = 1. I am not able to see any problem with it, but why to mark it as valid when actually there is no valid index. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bugs in CREATE/DROP INDEX CONCURRENTLY
On 2012-11-29 16:18:07 +0530, Amit Kapila wrote: On Thursday, November 29, 2012 12:39 AM Tom Lane wrote. Andres Freund and...@2ndquadrant.com writes: On 2012-11-27 23:46:58 -0500, Tom Lane wrote: Attached is a very preliminary draft patch for this. I've not addressed the question of whether we can clear indcheckxmin during transactional updates of pg_index rows, but I think it covers everything else talked about in this thread. Attached is an updated patch for HEAD that I think is about ready to go. I'll start making a back-patchable version shortly. I had verified in the Patch committed that the problem is resolved. I have a doubt related to RelationGetIndexList() function. In while loop, if index is not live then it continues, so it can be possible that we don't find a valid index after this while loop. But still after the loop, it marks relation-rd_indexvalid = 1. I am not able to see any problem with it, but why to mark it as valid when actually there is no valid index. rd_indexvalid is just saying whether rd_indexlist is valid. A zero element list seems to be valid to me. If we don't set rd_indexvalid pg_index will constantly be rescanned because the result isn't considered cached anymore. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bugs in CREATE/DROP INDEX CONCURRENTLY
On Thursday, November 29, 2012 4:24 PM Andres Freund wrote: On 2012-11-29 16:18:07 +0530, Amit Kapila wrote: On Thursday, November 29, 2012 12:39 AM Tom Lane wrote. Andres Freund and...@2ndquadrant.com writes: On 2012-11-27 23:46:58 -0500, Tom Lane wrote: Attached is a very preliminary draft patch for this. I've not addressed the question of whether we can clear indcheckxmin during transactional updates of pg_index rows, but I think it covers everything else talked about in this thread. Attached is an updated patch for HEAD that I think is about ready to go. I'll start making a back-patchable version shortly. I had verified in the Patch committed that the problem is resolved. I have a doubt related to RelationGetIndexList() function. In while loop, if index is not live then it continues, so it can be possible that we don't find a valid index after this while loop. But still after the loop, it marks relation-rd_indexvalid = 1. I am not able to see any problem with it, but why to mark it as valid when actually there is no valid index. rd_indexvalid is just saying whether rd_indexlist is valid. A zero element list seems to be valid to me. If we don't set rd_indexvalid pg_index will constantly be rescanned because the result isn't considered cached anymore. Got the point. Thanks. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Patch to fix ecpg core dump with nested structure pointer variable
Hi I have bumped in the segmentation fault in ecpg when using pointer to nested structure variable. ecpg crash.pgc Segmentation fault: 11 Please see the attached patch for the fix and test case to reproduce the scenario. Thanks Regards Muhammad Usama crash.pgc Description: Binary data ecpg_crash.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] WIP: index support for regexp search
One thing that bothers me with this algoritm is that the overflow
mechanism is all-or-nothing. In many cases, even when there is a huge
number of states in the diagram, you could still extract at least a few
trigrams that must be present in any matching string, with little
effort. At least, it seems like that to a human :-).
For example, consider this:
explain analyze select count(*) from azjunk4 where txt ~
('^aabaacaadaaeaafaagaahaaiaajaakaalaamaanaaoaapaaqaaraasaataauaavaawaaxaayaazabaabbabcabdabeabfabgabhabiabjabkablabmabnaboabpabqabrabsabtabuabvabwabxabyabzacaacbaccacdaceacfacgachaciacjackaclacmacnacoacpacqacracsactacuacvacwacxacyaczadaadbadcaddadeadfadgadhadiadjadkadladmadnadoadpadqadradsadtaduadvadwadxadyadzaeaaebaecaedaeeaefaegaehaeiaejaekaelaemaenaeoaepaeqaeraesaetaeuaevaewaexaeyaezafaafbafcafdafeaffafgafhafiafjafkaflafmafnafoafpafqafrafsaftafuafvafwafxafyafzagaagbagcagdageagfaggaghagiagjagkaglagmagnagoagpagqagragsagtaguagvagwagxagyagzahaahbahcahdaheahfahgahhahiahjahkahlahmahnahoahpahqahrahs$');
you get a query plan like this (the long regexp string edited out):
Aggregate (cost=228148.02..228148.03 rows=1 width=0) (actual
time=131.100..131
.101 rows=1 loops=1)
- Bitmap Heap Scan on azjunk4 (cost=228144.01..228148.02 rows=1
width=0) (
actual time=131.096..131.096 rows=0 loops=1)
Recheck Cond: (txt ~ ridiculously long regexp)
Rows Removed by Index Recheck: 1
- Bitmap Index Scan on azjunk4_trgmrgx_txt_01_idx
(cost=0.00..228144
.01 rows=1 width=0) (actual time=82.914..82.914 rows=1 loops=1)
Index Cond: (txt ~ ridiculously long regexp)
Total runtime: 131.230 ms
(7 rows)
That ridiculously long string exceeds the number of states (I think,
could be number of paths or arcs too), and the algorithm gives up,
resorting to scanning the whole index as can be seen by the Rows
Removed by Index Recheck line. However, it's easy to see that any
matching string must contain *any* of the possible trigrams the
algorithm extracts. If it could safely return just a few of them, say
aab and abz, and discard the rest, that would already be much better
than a full index scan.
Would it be safe to simply stop short the depth-first search on
overflow, and proceed with the graph that was constructed up to that point?
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] make -jN check fails / unset MAKEFLAGS in pg_regress.c
Hi, Currently make -jN check fails during creating temporary installation with: make[1]: *** read jobs pipe: Invalid argument. Stop. make[1]: *** Waiting for unfinished jobs make[2]: warning: jobserver unavailable: using -j1. Add `+' to parent make rule. in install.log. This is due to pg_regress invoking make while being invoked by make itself. gnu make internally sets the MAKEFLAGS environment variable to remember arguments. The problem in this case is that it contains --jobserver-fds=4,5 which makes the pg_regress invoked make think its running as a make child process. Now the problem obviously can be worked around by using make -jN make check instead of make -j16 check but I several times now have spent time trying to figure out what I broke so it sees sensible to fix this. Any arguments against doing so? The attached patch also resets the MAKELEVEL environment variable for good measure. I haven't seen any indication that its needed, but it feelds safer ;) Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services From 53d560d707964017ec0ef8cdd9d4a00632f3feec Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Thu, 29 Nov 2012 14:49:42 +0100 Subject: [PATCH] Unset MAKEFLAGS in pg_regress.c to hide the knowledge that its invoked by make from submakes Make stores some flags in the MAKEFLAGS variable to pass arguments to its own children. If we are invoked by make that makes the make invoked by us think its part of the parallel make invoking us and tries to communicate with the toplevel make. Which fails. --- src/test/regress/pg_regress.c | 12 1 file changed, 12 insertions(+) diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 81e7b69..1a25252 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -783,6 +783,18 @@ initialize_environment(void) } /* + * make stores some flags in the MAKEFLAGS variable to pass arguments + * to its own children. If we are invoked by make that makes the make + * invoked by us think its part of the parallel make invoking us and + * tries to communicate with the toplevel make. Which fails. + * + * Unset the variable to protect against such problems. We also reset + * MAKELEVEL to be certain the child notice the make above us. + */ + unsetenv(MAKEFLAGS); + unsetenv(MAKELEVEL); + + /* * Adjust path variables to point into the temp-install tree */ tmp = malloc(strlen(temp_install) + 32 + strlen(bindir)); -- 1.7.12.289.g0ce9864.dirty -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] json accessors
On 11/28/2012 08:16 PM, Hannu Krosing wrote:
On 11/29/2012 02:07 AM, Hannu Krosing wrote:
On 11/29/2012 01:10 AM, Merlin Moncure wrote:
On Wed, Nov 28, 2012 at 2:44 PM, Andrew Dunstan
and...@dunslane.net wrote:
...
*) have you considered something like
anyelement from_json(anyelement, json)
or
select json::some_type; (this may or many not be possible given
our
casting mechanics; i don't know).
I have no idea what the semantics of this would be.
Yeah, there's a lot of nuance there.
One way to tackle it would give the argument element as a template
and the result will the same template filled in from json filled
create table tab1(id serial primary key, ts timestamp default now(),
data text);
insert into tab1 select from_json(row(null,null,null)::tab1,
'{data:the data}');
insert into tab1 select from_json(row(null,null,null)::tab1,
'{id:-1, ts:null, data:}');
insert into tab1 select from_json(t.*,'{data:more data}') from
tab1 t where id = -1;
hannu=# select row_to_json(t.*) from tab1 t;
row_to_json
---
{id:1,ts:2012-11-29 02:01:48.379172,data:the data}
{id:-1,ts:null, data:}
{id:2,ts:2012-11-29 02:02:34.600164,data:more data}
(3 rows)
if extracting the defaults from table def proves too tricky for first
iteration, then
just set the missing fields to NULL or even better, carry over the
values from template;
You could even do a template-less row_from_json which returns a
records with all fields converted to
the JSON-encodable types and hope that the next conversions will be
done by postgreSQL as needed.
insert into tab1 select row_from_json('{id:100, ts:2012-12-21,
data:End of Everything}');
insert into tab1
select * from row_from_json(
'[{id:101, ts:2012-12-22, data:1st day after End of Everything}
{id:102, ts:2012-12-22, data:2nd day after End of Everything}
]');
The real problem here is that for any irregularly shaped json it's
likely to be a bust, and could only possibly work sanely for nested json
at all if the target type had corresponding array and composite fields.
hstore's populate_record works fairly well precisely because hstore is a
flat structure, unlike json.
In any case, I think this sort of suggestion highlights the possible
benefits of what I suggested upthread, namely to expose an API that will
allow easy construction of json transformation functions as extensions.
PS: good work so far :)
Hannu
Thanks.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Refactor flex and bison make rules
On Wed, 28 Nov 2012, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 11/28/2012 02:14 PM, Alvaro Herrera wrote: Okapi has been failing sporadically on ecpg, and I wonder if it's related to this change. Well, it looks like the make is broken and missing a clear dependency requirement. I think we need to ask Jeremy to turn off parallel build for okapi. Yeah, we already know that unpatched make 3.82 has got serious parallelism bugs: http://archives.postgresql.org/pgsql-hackers/2012-09/msg00397.php I wonder whether adding another .NOTPARALLEL directive would be a better idea than insisting people get hold of patched versions. While we're talking about odd issues that only seem to happen on Okapi, does anyone know of anything I can do to diagnose the pg_upgrade failure on the 9.2 branch? There are no rogue (non-buildfarm-related) postmaster/postgres processes running on the machine. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Refactor flex and bison make rules
On Wed, 28 Nov 2012, Tom Lane wrote: Jeremy Drake pgbuildf...@jdrake.com writes: While we're talking about odd issues that only seem to happen on Okapi, does anyone know of anything I can do to diagnose the pg_upgrade failure on the 9.2 branch? There are no rogue (non-buildfarm-related) postmaster/postgres processes running on the machine. [ digs around ... ] It looks like the failure is coming from here: if (strlen(path) = sizeof(unp-sun_path)) return EAI_FAIL; What's the size of the sun_path member of struct sockaddr_un on your machine? I count 115 characters in your socket path ... maybe you just need a less deeply nested test directory. (If that is the problem, seems like we need to return something more helpful than EAI_FAIL here.) /usr/include/sys/un.h:char sun_path[108]; /* Path name. */ That seems to be it. This may be just the excuse I needed to set up dedicated users for my buildfarm animals. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] autovacuum truncate exclusive lock round two
On 11/28/2012 3:33 PM, Kevin Grittner wrote: Kevin Grittner wrote: I still need to review the timing calls, since I'm not familiar with them so it wasn't immediately obvious to me whether they were being used correctly. I have no reason to believe that they aren't, but feel I should check. It seems odd to me that assignment of one instr_time to another is done with INSTR_TIME_SET_ZERO() of the target followed by INSTR_TIME_ADD() with the target and the source. It seems to me that simple assignment would be more readable, and I can't see any down side. Why shouldn't: INSTR_TIME_SET_ZERO(elapsed); INSTR_TIME_ADD(elapsed, currenttime); INSTR_TIME_SUBTRACT(elapsed, starttime); instead be?: elapsed = currenttime; INSTR_TIME_SUBTRACT(elapsed, starttime); And why shouldn't: INSTR_TIME_SET_ZERO(starttime); INSTR_TIME_ADD(starttime, currenttime); instead be?: starttime = currenttime; Resetting starttime this way seems especially odd. instr_time is LARGE_INTEGER on Win32 but struct timeval on Unix. Is starttime = currenttime; portable if those are structs? Also, I want to do another pass looking just for off-by-one errors on blkno. Again, I have no reason to believe that there is a problem; it just seems like it would be a particularly easy type of mistake to make and miss when a key structure has this field: BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ No problems found with that. And I want to test more. The patch worked as advertised in all my tests, but I became uncomforatable with the games being played with the last autovacuum timestamp and the count of dead tuples. Sure, that causes autovacuum to kick back in until it has dealt with the truncation, but it makes it hard for a human looking at the stat views to see what's going on, and I'm not sure it wouldn't lead to bad plans due to stale statistics. Personally, I would rather see us add a boolean to indicate that autovacuum was needed (regardless of the math on the other columns) and use that to control the retries -- leaving the other columns free to reflect reality. You mean to extend the stats by yet another column? The thing is that this whole case happens rarely. We see this every other month or so and only on a rolling window table after it got severely bloated due to some abnormal use (purging disabled for some operational reason). The whole situation resolves itself after a few minutes to maybe an hour or two. Personally I don't think living with a few wrong stats on one table for that time is so bad that it warrants changing that much more code. Lower casing TRUE/FALSE will be done. If the LW_SHARED is enough in LockHasWaiters(), that's fine too. I think we have a consensus that the check interval should be derived from deadlock_timeout/10 clamped to 10ms. I'm going to remove that GUC. About the other two, I'm just not sure. Maybe using half the value as for the lock waiter interval as the lock retry interval, again clamped to 10ms, and simply leaving one GUC that controls how long autovacuum should retry this. I'm not too worried about the retry interval. However, the problem with that overall retry duration is that this value very much depends on the usage pattern of the database. If long running transactions (like 5 seconds) are the norm, then a hard coded value of 2 seconds before autovacuum gives up isn't going to help much. Jan -- Anyone who trades liberty for security deserves neither liberty nor security. -- Benjamin Franklin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] autovacuum truncate exclusive lock round two
Jan Wieck janwi...@yahoo.com writes: On 11/28/2012 3:33 PM, Kevin Grittner wrote: Resetting starttime this way seems especially odd. instr_time is LARGE_INTEGER on Win32 but struct timeval on Unix. Is starttime = currenttime; portable if those are structs? Sure. We rely on struct assignment in lots of places. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] autovacuum truncate exclusive lock round two
On 11/29/2012 9:46 AM, Tom Lane wrote: Jan Wieck janwi...@yahoo.com writes: On 11/28/2012 3:33 PM, Kevin Grittner wrote: Resetting starttime this way seems especially odd. instr_time is LARGE_INTEGER on Win32 but struct timeval on Unix. Is starttime = currenttime; portable if those are structs? Sure. We rely on struct assignment in lots of places. Will be done then. Thanks, Jan -- Anyone who trades liberty for security deserves neither liberty nor security. -- Benjamin Franklin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign key locks
Kevin Grittner wrote: Kevin Grittner wrote: Alvaro Herrera wrote: Here's version 24. This no longer applies after the rmgr rm_desc patch. I took a shot at merging those so I could review the patch against HEAD; attached in hopes that it will be useful. Hopefully this isn't too far off from what . Uh, sorry for remaining silent -- I'm about to post an updated patch, having just pushed my merge to github. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] binary heap implementation
On Wed, Nov 28, 2012 at 3:21 PM, Andres Freund and...@2ndquadrant.com wrote: Looks ready to me. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Refactor flex and bison make rules
On 11/28/2012 11:03 PM, Tom Lane wrote: Jeremy Drake pgbuildf...@jdrake.com writes: While we're talking about odd issues that only seem to happen on Okapi, does anyone know of anything I can do to diagnose the pg_upgrade failure on the 9.2 branch? There are no rogue (non-buildfarm-related) postmaster/postgres processes running on the machine. [ digs around ... ] It looks like the failure is coming from here: if (strlen(path) = sizeof(unp-sun_path)) return EAI_FAIL; What's the size of the sun_path member of struct sockaddr_un on your machine? I count 115 characters in your socket path ... maybe you just need a less deeply nested test directory. (If that is the problem, seems like we need to return something more helpful than EAI_FAIL here.) Looks like it was. Good catch. What's the best way to fix? Note that this problem was triggered by having a buildfarm buildroot path of length about 49 or 50. I'm lucky not to have triggered it myself. Do I need to add a check to limit the buildroot path length? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to fix ecpg core dump with nested structure pointer variable
I have bumped in the segmentation fault in ecpg when using pointer to nested structure variable. ... Please see the attached patch for the fix and test case to reproduce the scenario. Thanks for spotting and fixing this. Patch committed to git. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/28/2012 11:03 PM, Tom Lane wrote: [ digs around ... ] It looks like the failure is coming from here: if (strlen(path) = sizeof(unp-sun_path)) return EAI_FAIL; Looks like it was. Good catch. What's the best way to fix? So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. Another line of attack is to just teach getnameinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. The portability risk here is if sun_path is not the last field in struct sockaddr_un on some platform --- but that seems a bit unlikely, and even if it isn't I doubt we access any other members besides sun_family and sun_path. I kind of like this approach, since it gets rid of the length limitation rather than just reporting it more clearly. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bugs in CREATE/DROP INDEX CONCURRENTLY
And here is a version for 9.1. This omits the code changes directly
relevant to DROP INDEX CONCURRENTLY, but includes the changes to avoid
transactional updates of the pg_index row during CREATE CONCURRENTLY,
as well as the changes to prevent use of not-valid or not-ready indexes
in places where it matters. I also chose to keep on using the
IndexIsValid and IndexIsReady macros, so as to avoid unnecessary
divergences of the branches.
I think this much of the patch needs to go into all supported branches.
regards, tom lane
diff --git a/src/backend/access/heap/README.HOT b/src/backend/access/heap/README.HOT
index f12cad44e56c363e62fa617497bbedfe1ba8c1fe..7cbc6a1d7e7328364938ef5d69bebe865524d7f8 100644
*** a/src/backend/access/heap/README.HOT
--- b/src/backend/access/heap/README.HOT
*** from the index, as well as ensuring that
*** 386,391
--- 386,397
rows in a broken HOT chain (the first condition is stronger than the
second). Finally, we can mark the index valid for searches.
+ Note that we do not need to set pg_index.indcheckxmin in this code path,
+ because we have outwaited any transactions that would need to avoid using
+ the index. (indcheckxmin is only needed because non-concurrent CREATE
+ INDEX doesn't want to wait; its stronger lock would create too much risk of
+ deadlock if it did.)
+
Limitations and Restrictions
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 2e023d5ab56ac4efe1ab243739b48149e67a7408..129a1ac11f0e79408961abd73f0a33395569c5d5 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
*** static void ResetReindexPending(void);
*** 129,134
--- 129,138
* See whether an existing relation has a primary key.
*
* Caller must have suitable lock on the relation.
+ *
+ * Note: we intentionally do not check IndexIsValid here; that's because this
+ * is used to enforce the rule that there can be only one indisprimary index,
+ * and we want that to be true even if said index is invalid.
*/
static bool
relationHasPrimaryKey(Relation rel)
*** index_constraint_create(Relation heapRel
*** 1247,1254
* Note: since this is a transactional update, it's unsafe against
* concurrent SnapshotNow scans of pg_index. When making an existing
* index into a constraint, caller must have a table lock that prevents
! * concurrent table updates, and there is a risk that concurrent readers
! * of the table will miss seeing this index at all.
*/
if (update_pgindex (mark_as_primary || deferrable))
{
--- 1251,1259
* Note: since this is a transactional update, it's unsafe against
* concurrent SnapshotNow scans of pg_index. When making an existing
* index into a constraint, caller must have a table lock that prevents
! * concurrent table updates; if it's less than a full exclusive lock,
! * there is a risk that concurrent readers of the table will miss seeing
! * this index at all.
*/
if (update_pgindex (mark_as_primary || deferrable))
{
*** BuildIndexInfo(Relation index)
*** 1450,1456
/* other info */
ii-ii_Unique = indexStruct-indisunique;
! ii-ii_ReadyForInserts = indexStruct-indisready;
/* initialize index-build state to default */
ii-ii_Concurrent = false;
--- 1455,1461
/* other info */
ii-ii_Unique = indexStruct-indisunique;
! ii-ii_ReadyForInserts = IndexIsReady(indexStruct);
/* initialize index-build state to default */
ii-ii_Concurrent = false;
*** index_build(Relation heapRelation,
*** 1789,1796
* index's usability horizon. Moreover, we *must not* try to change the
* index's pg_index entry while reindexing pg_index itself, and this
* optimization nicely prevents that.
*/
! if (indexInfo-ii_BrokenHotChain !isreindex)
{
Oid indexId = RelationGetRelid(indexRelation);
Relation pg_index;
--- 1794,1813
* index's usability horizon. Moreover, we *must not* try to change the
* index's pg_index entry while reindexing pg_index itself, and this
* optimization nicely prevents that.
+ *
+ * We also need not set indcheckxmin during a concurrent index build,
+ * because we won't set indisvalid true until all transactions that care
+ * about the broken HOT chains are gone.
+ *
+ * Therefore, this code path can only be taken during non-concurrent
+ * CREATE INDEX. Thus the fact that heap_update will set the pg_index
+ * tuple's xmin doesn't matter, because that tuple was created in the
+ * current transaction anyway. That also means we don't need to worry
+ * about any concurrent readers of the tuple; no other transaction can see
+ * it yet.
*/
! if (indexInfo-ii_BrokenHotChain !isreindex
! !indexInfo-ii_Concurrent)
{
Oid indexId = RelationGetRelid(indexRelation);
Relation pg_index;
***
Re: [HACKERS] Further pg_upgrade analysis for many tables
On Wed, Nov 28, 2012 at 03:22:32PM -0500, Bruce Momjian wrote:
On Tue, Nov 27, 2012 at 09:35:10PM -0800, Jeff Janes wrote:
I tested custom format with pg_restore -j and -1, as well as text
restore. The winner was pg_dump -Fc | pg_restore -1;
I don't have the numbers at hand, but if my relcache patch is
accepted, then -1 stops being faster.
-1 gets rid of the AtOEXAct relcache N^2 behavior, but at the cost of
invoking a different N^2, that one in the stats system.
OK, here are the testing results:
#tbls git -1AtOEXAct both
1 11.06 13.06 10.99 13.20
1000 21.71 22.92 22.20 22.51
2000 32.86 31.09 32.51 31.62
4000 55.22 49.96 52.50 49.99
8000 105.34 82.10 95.32 82.94
16000 223.67 164.27 187.40 159.53
32000 543.93 324.63 366.44 317.93
640001697.14 791.82 767.32 752.57
Up to 2k, they are all similar. 4k 8k have the -1 patch as a win, and
16k+ really need both patches.
I will continue working on the -1 patch, and hopefully we can get your
AtOEXAct patch in soon. Is someone reviewing that?
I have polished up the patch (attached) and it is ready for application
to 9.3.
Since there is no pg_dump/pg_restore pipe parallelism, I had the old
cluster create per-database dump files, so I don't need to have the old
and new clusters running at the same time, which would have required two
port numbers and make shared memory exhaustion more likely.
We now create a dump file per database, so thousands of database dump
files might cause a performance problem.
This also adds status output so you can see the database names as their
schemas are dumped and restored. This was requested by users.
I retained custom mode for pg_dump because it is measurably faster than
text mode (not sure why, psql overhead?):
git -Fc -Fp
1 11.04 11.08 11.02
1000 22.37 19.68 21.64
2000 32.39 28.62 31.40
4000 56.18 48.53 51.15
8000 105.15 81.23 91.84
16000 227.64 156.72 177.79
32000 542.80 323.19 371.81
640001711.77 789.17 865.03
Text dump files are slightly easier to debug, but probably not by much.
Single-transaction restores were recommended to me over a year ago (by
Magnus?), but I wanted to get pg_upgrade rock-solid before doing
optimization, and now is the right time to optimize.
One risk of single-transaction restores is max_locks_per_transaction
exhaustion, but you will need to increase that on the old cluster for
pg_dump anyway because that is done a single transaction, so the only
new thing is that the new cluster might also need to adjust
max_locks_per_transaction.
I was able to remove split_old_dump() because pg_dumpall now produces a
full global restore file and we do database dumps separately.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 285f10c..bccceb1
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** output_check_banner(bool *live_check)
*** 72,78
void
! check_old_cluster(bool live_check, char **sequence_script_file_name)
{
/* -- OLD -- */
--- 72,78
void
! check_and_dump_old_cluster(bool live_check, char **sequence_script_file_name)
{
/* -- OLD -- */
*** check_old_cluster(bool live_check, char
*** 131,140
* the old server is running.
*/
if (!user_opts.check)
- {
generate_old_dump();
- split_old_dump();
- }
if (!live_check)
stop_postmaster(false);
--- 131,137
diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
new file mode 100644
index 577ccac..d206e98
*** a/contrib/pg_upgrade/dump.c
--- b/contrib/pg_upgrade/dump.c
***
*** 16,110
void
generate_old_dump(void)
{
! /* run new pg_dumpall binary */
! prep_status(Creating catalog dump);
! /*
! * --binary-upgrade records the width of dropped columns in pg_class, and
! * restores the frozenid's for databases and relations.
! */
exec_prog(UTILITY_LOG_FILE, NULL, true,
! \%s/pg_dumpall\ %s --schema-only --binary-upgrade %s -f %s,
new_cluster.bindir, cluster_conn_opts(old_cluster),
log_opts.verbose ? --verbose : ,
! ALL_DUMP_FILE);
! check_ok();
! }
!
!
! /*
! * split_old_dump
! *
! * This function splits pg_dumpall output into global values and
! * database creation, and per-db schemas. This allows us to create
! * the support functions between restoring these two parts of the
! * dump. We split on the first \connect after a CREATE ROLE
! * username match; this is where the per-db restore
Re: [HACKERS] json accessors
On Thu, Nov 29, 2012 at 7:58 AM, Andrew Dunstan and...@dunslane.net wrote:
On 11/28/2012 08:16 PM, Hannu Krosing wrote:
You could even do a template-less row_from_json which returns a records
with all fields converted to
the JSON-encodable types and hope that the next conversions will be done
by postgreSQL as needed.
insert into tab1 select row_from_json('{id:100, ts:2012-12-21,
data:End of Everything}');
insert into tab1
select * from row_from_json(
'[{id:101, ts:2012-12-22, data:1st day after End of Everything}
{id:102, ts:2012-12-22, data:2nd day after End of Everything}
]');
The real problem here is that for any irregularly shaped json it's likely to
be a bust, and could only possibly work sanely for nested json at all if the
target type had corresponding array and composite fields.
again, that's pretty a fairly typical case -- crafting json documents
specifically for consumption in postgres. defining backend types
allows you to skip intermediate iterative marshaling step.
hstore's
populate_record works fairly well precisely because hstore is a flat
structure, unlike json.
agreed. not trying to drag you into the weeds here. the above is neat
functionality but doesn't cover all the cases so specific accessor
functions in the vein of your proposal are still needed and the hstore
workaround should work pretty well -- sugaring up the syntax for 'all
in wonder' type translations of complicated structures can be done
later if you want to keep things simple in the short term.
so, just hashing out your proposal and making sure it's reasonable
analogous implementation of xpath. Sleeping on it, I say mostly, but
not quite. how about some changes for json_get:
1) return setof (key, value) in the style of jquery each().
2) we need some way of indicating in the keytext path that we want to
unnest the collecton pointed to by keytext or to just return it. for
example, -* as indicator?
3) use double quotes, and make them optional (as hstore)
4) speaking of hstore, prefer = vs -?
if you do at least #1 and #2, json_get I think can cover all the bases
for parsing json, meaning you could reproduce the behaviors for each
of your four proposed just as xpath does for xml. (you may still
want to add them for posterity or performance though). so no need for
json_each or json_array_unnest etc.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] json accessors
On 11/29/2012 01:06 PM, Merlin Moncure wrote: so, just hashing out your proposal and making sure it's reasonable analogous implementation of xpath. Sleeping on it, I say mostly, but not quite. how about some changes for json_get: 1) return setof (key, value) in the style of jquery each(). 2) we need some way of indicating in the keytext path that we want to unnest the collecton pointed to by keytext or to just return it. for example, -* as indicator? 3) use double quotes, and make them optional (as hstore) 4) speaking of hstore, prefer = vs -?So I don't think your modifications are well thought out. if you do at least #1 and #2, json_get I think can cover all the bases for parsing json, meaning you could reproduce the behaviors for each of your four proposed just as xpath does for xml. (you may still want to add them for posterity or performance though). so no need for json_each or json_array_unnest etc. json_get is designed to return a single thing. What is more, returning a (key, value) pair seems quite silly when you're passing the key in as an argument. It's not designed to be json_path or json_query, and it's not designed either to take a path expression as an argument. So I don't think this is a good direction. Your proposed mods to json_get modify it out of all recognition. If I offer you a horse and ask what colour you'd like, asking for a lion instead isn't a good response :-) (Repeating myself), I also suggest exposing the transform API so that it will be easy to construct further functions as extensions. I'm not trying to cover the field. The intention here is to provide some very basic json accessors as core functions / operators. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign key locks - EvalPlanQual interactions
On 2012-11-27 12:07:34 -0300, Alvaro Herrera wrote: Alvaro Herrera wrote: Here's version 24. Old review emails still contains some items that didn't lead to any changes. I tried to keep close track of those. To that list I add a couple of things of my own. Here they are, for those following along. I welcome suggestions. - EvalPlanQual and ExecLockRows maybe need a different overall locking strategy. Right now follow_updates=false is used to avoid deadlocks. I think this really might need some work. Afaics the EPQ code now needs to actually determine what locklevel needs to be passed to EvalPlanQualFetch via EvalPlanQual otherwise some of the locking issues that triggered all this remain. That sucks a bit from a modularity perspective, but I don't see another way. Greetings, Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bugs in CREATE/DROP INDEX CONCURRENTLY
On 2012-11-29 11:53:50 -0500, Tom Lane wrote: And here is a version for 9.1. This omits the code changes directly relevant to DROP INDEX CONCURRENTLY, but includes the changes to avoid transactional updates of the pg_index row during CREATE CONCURRENTLY, as well as the changes to prevent use of not-valid or not-ready indexes in places where it matters. I also chose to keep on using the IndexIsValid and IndexIsReady macros, so as to avoid unnecessary divergences of the branches. Looks good me. I think this much of the patch needs to go into all supported branches. Looks like that to me, yes. Thanks for all that work! Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enabling frontend-only xlog desc routines
Alvaro Herrera alvhe...@2ndquadrant.com writes: The other interesting question remaining is what to do about the rm_desc function in rmgr.c. We're split between these two ideas: Why try to link rmgr.c into frontend versions at all? Just make a new table file that only includes the desc function pointers. Yeah, then there would be two table files to maintain, but it's not clear to me that it's uglier than these proposals ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enabling frontend-only xlog desc routines
On 2012-11-29 15:03:48 -0500, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: The other interesting question remaining is what to do about the rm_desc function in rmgr.c. We're split between these two ideas: Why try to link rmgr.c into frontend versions at all? Just make a new table file that only includes the desc function pointers. Yeah, then there would be two table files to maintain, but it's not clear to me that it's uglier than these proposals ... Seems more likely to get out of sync. But then, rmgr.c isn't changing that heavily... I still prefer the -DFRONTEND solutions, but once more, its only a slight preference. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
I wrote: Andrew Dunstan and...@dunslane.net writes: Looks like it was. Good catch. What's the best way to fix? So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. I tried this out and found that at least on Linux, gai_strerror() is too stupid to pay attention to errno anyway; you just get System error, which is about as unhelpful as it could possibly be. I don't see any way that we can get a more specific error message to be printed without eliminating use of gai_strerror and providing our own infrastructure for reporting getaddrinfo errors. While that wouldn't be incredibly awful (we have such infrastructure already for ancient platforms...), it still kinda sucks. Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. I'm a bit tempted to just replace EAI_FAIL with EAI_MEMORY here, so that you'd get messages similar to Memory allocation failure. That might at least point your thoughts in the right direction, whereas Non-recoverable failure in name resolution certainly conveys nothing of use. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 03:33 PM, Tom Lane wrote: I wrote: Andrew Dunstan and...@dunslane.net writes: Looks like it was. Good catch. What's the best way to fix? So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. I tried this out and found that at least on Linux, gai_strerror() is too stupid to pay attention to errno anyway; you just get System error, which is about as unhelpful as it could possibly be. I don't see any way that we can get a more specific error message to be printed without eliminating use of gai_strerror and providing our own infrastructure for reporting getaddrinfo errors. While that wouldn't be incredibly awful (we have such infrastructure already for ancient platforms...), it still kinda sucks. Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. I'm a bit tempted to just replace EAI_FAIL with EAI_MEMORY here, so that you'd get messages similar to Memory allocation failure. That might at least point your thoughts in the right direction, whereas Non-recoverable failure in name resolution certainly conveys nothing of use. Thoughts? I don't think it's worth a heroic effort. Meanwhile I'll add a check in the upgrade test module(s) to check for overly long paths likely to give problems. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?
Hi,
while looking at trigger.c from the POV of foreign key locks I noticed
that GetTupleForTrigger commentless assumes it can just look at a pages
content without a
LockBuffer(buffer, BUFFER_LOCK_SHARE);
That code path is only reached for AFTER ... FOR EACH ROW ... triggers,
so its fine it's not locking the tuple itself. That already needs to
have happened before.
The code in question:
if (newslot_which_is_passed_by_before_triggers)
...
else
{
Pagepage;
ItemId lp;
buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));
page = BufferGetPage(buffer);
lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
Assert(ItemIdIsNormal(lp));
tuple.t_data = (HeapTupleHeader) PageGetItem(page, lp);
tuple.t_len = ItemIdGetLength(lp);
tuple.t_self = *tid;
tuple.t_tableOid = RelationGetRelid(relation);
}
result = heap_copytuple(tuple);
ReleaseBuffer(buffer);
As can be seen in the excerpt above this is basically a very stripped
down heap_fetch(). But without any locking on the buffer we just read.
I can't believe this is safe? Just about anything but eviction could
happen to that buffer at that moment?
Am I missing something?
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] json accessors
On Thu, Nov 29, 2012 at 1:19 PM, Andrew Dunstan and...@dunslane.net wrote:
On 11/29/2012 01:06 PM, Merlin Moncure wrote:
so, just hashing out your proposal and making sure it's reasonable
analogous implementation of xpath. Sleeping on it, I say mostly, but
not quite. how about some changes for json_get:
1) return setof (key, value) in the style of jquery each().
2) we need some way of indicating in the keytext path that we want to
unnest the collecton pointed to by keytext or to just return it. for
example, -* as indicator?
3) use double quotes, and make them optional (as hstore)
4) speaking of hstore, prefer = vs -?So I don't think your modifications
are well thought out.
if you do at least #1 and #2, json_get I think can cover all the bases
for parsing json, meaning you could reproduce the behaviors for each
of your four proposed just as xpath does for xml. (you may still
want to add them for posterity or performance though). so no need for
json_each or json_array_unnest etc.
json_get is designed to return a single thing. What is more, returning a
(key, value) pair seems quite silly when you're passing the key in as an
argument. It's not designed to be json_path or json_query, and it's not
designed either to take a path expression as an argument. So I don't think
this is a good direction. Your proposed mods to json_get modify it out of
all recognition. If I offer you a horse and ask what colour you'd like,
asking for a lion instead isn't a good response :-)
(Repeating myself), I also suggest exposing the transform API so that it
will be easy to construct further functions as extensions. I'm not trying to
cover the field. The intention here is to provide some very basic json
accessors as core functions / operators.
Right. But you're not offering a horse to the farm...but to the zoo.
json is in core so I don't think you have the luxury of offering a
clunky API now withe expectation of a sleeker, faster one in the
future as the old functions will sit around forever in the public
namespace. What is present in the API doesn't have to cover all
reasonable use cases but it certainly should be expected withstand the
test of time for the cases it does cover.
Sketch out how a object array of indeterminate size would be parsed
and placed into records with a set returning/array returning and
non-set returning json_get: which is a better fit? xpath() doesn't
work iteratively and nobody has ever complained about that to my
recollection.
table: create table foo (a int, b int);
document: [{a: 1, b: 2}, {a: 3, b: 4}, ... {a: 9, b: 10}]
set returning json_get:
INSERT INTO foo
SELECT * FROM populate_record(null, hstore_to_json((json_get(*)).value));
assuming '*' is the 'expand this' operator in your 'keytext'
expression that I was suggestion. How would this work with your
proposed API? This is a very typical use case.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 03:33 PM, Tom Lane wrote: Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. I don't think it's worth a heroic effort. Meanwhile I'll add a check in the upgrade test module(s) to check for overly long paths likely to give problems. I'm thinking maybe we should try to fix this. What's bugging me is that Jeremy's build path doesn't look all that unreasonably long. The scary scenario that's in the back of my mind is that one day somebody decides to rearrange the Red Hat build servers a bit and suddenly I can't build Postgres there anymore, because the build directory is nested a bit too deep. Murphy's law would of course dictate that I find this out only when under the gun to get a security update packaged. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On Thu, Nov 29, 2012 at 03:33:59PM -0500, Tom Lane wrote: I wrote: So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. I tried this out and found that at least on Linux, gai_strerror() is too stupid to pay attention to errno anyway; you just get System error, which is about as unhelpful as it could possibly be. I don't see any way that we can get a more specific error message to be printed without eliminating use of gai_strerror and providing our own infrastructure for reporting getaddrinfo errors. While that wouldn't be incredibly awful (we have such infrastructure already for ancient platforms...), it still kinda sucks. RFC 2553 and successor standards do not call for gai_strerror() to look at anything other than its argument, so your finding for Linux surprises me less than its alternative. Adopt code like rc == EAI_SYSTEM ? strerror(errno) : gai_strerror(rc) to report the error, and your proposal to use ENAMETOOLONG sounds suitable. Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. Linux enforces a hard limit matching the static buffer in sockaddr_un. You'd proceed a bit further and hit could not bind Unix socket: Invalid argument or some such. I agree we should perhaps fix pg_upgrade to work even when its CWD is not usable as a socket path. It could create a temporary directory under /tmp and place the socket there, for example. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Noah Misch n...@leadboat.com writes:
Linux enforces a hard limit matching the static buffer in sockaddr_un. You'd
proceed a bit further and hit could not bind Unix socket: Invalid argument
or some such.
Hm, I was wondering about that. The Single Unix Spec suggests that
bind/connect ought to accept pathnames at least up to PATH_MAX, but
if popular implementations don't honor that then it is a bit pointless
for us to do a lot of pushups in userspace.
I agree we should perhaps fix pg_upgrade to work even when its CWD is not
usable as a socket path. It could create a temporary directory under /tmp and
place the socket there, for example.
Yeah, I was starting to think that pg_upgrade's test script is the real
culprit here. Every other variant of make check just puts the socket
in the default place, typically /tmp, so it's rather useless that this
one place is doing things differently.
Another thing that we should possibly consider if we're going to hack on
that is that make check is not currently very friendly to people who
try to move the default socket location to someplace other than /tmp,
such as the ever-popular /var/run/postgresql. The reason that this is
problematic is that /var/run/postgresql may not be there at all in a
build environment, and if it is, it's likely not writable by the user
you're running your build as. So just using the default socket
directory isn't real friendly in any case. In converting the Fedora
packages to use /var/run/postgresql recently, I found I had to add the
attached crude hacks to support running the regression tests during
build. It'd be nice if the consideration were handled by unmodified
sources ...
regards, tom lane
diff -Naur postgresql-9.2.0.sockets/contrib/pg_upgrade/test.sh
postgresql-9.2.0/contrib/pg_upgrade/test.sh
--- postgresql-9.2.0.sockets/contrib/pg_upgrade/test.sh 2012-09-06
17:26:17.0 -0400
+++ postgresql-9.2.0/contrib/pg_upgrade/test.sh 2012-09-06 18:13:18.178092176
-0400
@@ -62,10 +62,14 @@
rm -rf $logdir
mkdir $logdir
+# we want the Unix sockets in $temp_root
+PGHOST=$temp_root
+export PGHOST
+
set -x
$oldbindir/initdb
-$oldbindir/pg_ctl start -l $logdir/postmaster1.log -w
+$oldbindir/pg_ctl start -l $logdir/postmaster1.log -o -c
unix_socket_directories='$PGHOST' -w
if $MAKE -C $oldsrc installcheck; then
pg_dumpall -f $temp_root/dump1.sql || pg_dumpall1_status=$?
if [ $newsrc != $oldsrc ]; then
@@ -108,7 +112,7 @@
pg_upgrade -d ${PGDATA}.old -D ${PGDATA} -b $oldbindir -B $bindir
-pg_ctl start -l $logdir/postmaster2.log -w
+pg_ctl start -l $logdir/postmaster2.log -o -c
unix_socket_directories='$PGHOST' -w
if [ $testhost = Msys ] ; then
cmd /c analyze_new_cluster.bat
diff -Naur postgresql-9.2.0.sockets/src/test/regress/pg_regress.c
postgresql-9.2.0/src/test/regress/pg_regress.c
--- postgresql-9.2.0.sockets/src/test/regress/pg_regress.c 2012-09-06
17:26:17.0 -0400
+++ postgresql-9.2.0/src/test/regress/pg_regress.c 2012-09-06
18:13:18.184092537 -0400
@@ -772,7 +772,7 @@
if (hostname != NULL)
doputenv(PGHOST, hostname);
else
- unsetenv(PGHOST);
+ doputenv(PGHOST, /tmp);
unsetenv(PGHOSTADDR);
if (port != -1)
{
@@ -2233,7 +2233,7 @@
*/
header(_(starting postmaster));
snprintf(buf, sizeof(buf),
-SYSTEMQUOTE \%s/postgres\ -D \%s/data\
-F%s -c \listen_addresses=%s\ \%s/log/postmaster.log\ 21 SYSTEMQUOTE,
+SYSTEMQUOTE \%s/postgres\ -D \%s/data\
-F%s -c \listen_addresses=%s\ -c \unix_socket_directories=/tmp\
\%s/log/postmaster.log\ 21 SYSTEMQUOTE,
bindir, temp_install,
debug ? -d 5 : ,
hostname ? hostname : ,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 05:20 PM, Tom Lane wrote: I don't think it's worth a heroic effort. Meanwhile I'll add a check in the upgrade test module(s) to check for overly long paths likely to give problems. I'm thinking maybe we should try to fix this. What's bugging me is that Jeremy's build path doesn't look all that unreasonably long. The scary scenario that's in the back of my mind is that one day somebody decides to rearrange the Red Hat build servers a bit and suddenly I can't build Postgres there anymore, because the build directory is nested a bit too deep. Murphy's law would of course dictate that I find this out only when under the gun to get a security update packaged. The only thing it breaks AFAIK is pg_upgrade testing because pg_upgrade insists on setting the current directory as the socket dir. Maybe we need a pg_upgrade option to specify the socket dir to use. Or maybe the postmaster needs to check the length somehow before it calls StreamServerPort() so we can give a saner error message. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: ... Or maybe the postmaster needs to check the length somehow before it calls StreamServerPort() so we can give a saner error message. Hm. That's ugly, but a lot less invasive than trying to get the official API to pass the information through. Sounds like a plan to me. However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 06:23 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: ... Or maybe the postmaster needs to check the length somehow before it calls StreamServerPort() so we can give a saner error message. Hm. That's ugly, but a lot less invasive than trying to get the official API to pass the information through. Sounds like a plan to me. However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. The test script doesn't do anything. It's pg_upgrade itself that sets the socket dir. See option.c:get_sock_dir() cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] json accessors
On Thu, Nov 29, 2012 at 4:14 PM, Andrew Dunstan and...@dunslane.net wrote: There are many things wrong with this. First, converting to hstore so you can call populate_record is quite horrible and ugly and inefficient. And it's dependent on having hstore loaded - you can't have an hstore_to_jon in core because hstore itself isn't in core. If you want a populate_record that takes data from json we should have one coded direct. I'm happy to add it to the list as long as everyone understands the limitations. Given a function to unnest the json array, which I already suggested upthread, you could do what you suggested above much more elegantly and directly. I wasn't suggesting you added the hstore stuff and I understand perfectly well the awkwardness of the hstore route. That said, this is how people are going to use your api so it doesn't hurt to go through the motions; I'm just feeling out how code in the wild would shape up. Anyways, my example was busted since you'd need an extra step to move the set returning output from the json array unnest() into a 'populate_record' type function call. So, AIUI I think you're proposing (i'm assuming optional quotes) following my example above: INSERT INTO foo(a,b) SELECT json_get_as_text(v, 'a')::int, json_get_as_text(v, 'b')::int FROM json_each(document) v; /* gives you array of json (a,b) records */ a hypothetical 'json_to_record (cribbing usage from populate_record)' variant might look like (please note, I'm not saying 'write this now', just feeling it out):: INSERT INTO foo(a,b) SELECT r.* FROM json_each(document) v, LATERAL json_to_record(null::foo, v) r; you're right: that's pretty clean. An json_object_each(json), = key, value couldn't hurt either -- this would handle those oddball cases of really wide objects that you occasionally see in json. Plus as_text variants of both each and object_each. If you're buying json_object_each, I think you can scrap json_object_keys(). merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 06:23 PM, Tom Lane wrote: However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. The test script doesn't do anything. It's pg_upgrade itself that sets the socket dir. See option.c:get_sock_dir() Um ... that's *another* place that needs to change then, because the test script fires up postmasters on its own, outside of pg_upgrade. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Tom Lane t...@sss.pgh.pa.us writes:
Andrew Dunstan and...@dunslane.net writes:
... Or maybe the
postmaster needs to check the length somehow before it calls
StreamServerPort() so we can give a saner error message.
Hm. That's ugly, but a lot less invasive than trying to get the
official API to pass the information through. Sounds like a plan to me.
Here's a patch for that --- I think we should apply and back-patch this.
regards, tom lane
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 5e86987f221fee01c5049f925492e0f9c441d372..15a01a8324b8a6ff9727a1b02e7ba8fe385d3a39 100644
*** a/src/backend/libpq/pqcomm.c
--- b/src/backend/libpq/pqcomm.c
*** StreamServerPort(int family, char *hostN
*** 308,313
--- 308,321
* that file path
*/
UNIXSOCK_PATH(unixSocketPath, portNumber, unixSocketDir);
+ if (strlen(unixSocketPath) = UNIXSOCK_PATH_BUFLEN)
+ {
+ ereport(LOG,
+ (errmsg(Unix-domain socket path \%s\ is too long (maximum %d bytes),
+ unixSocketPath,
+ (int) (UNIXSOCK_PATH_BUFLEN - 1;
+ return STATUS_ERROR;
+ }
if (Lock_AF_UNIX(unixSocketDir, unixSocketPath) != STATUS_OK)
return STATUS_ERROR;
service = unixSocketPath;
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 604b5535df6b24885b782688fee9ff7fd284746b..635132dec9317b4045108c75cd8c67a70c360968 100644
*** a/src/include/libpq/pqcomm.h
--- b/src/include/libpq/pqcomm.h
*** typedef struct
*** 74,79
--- 74,92
(port))
/*
+ * The maximum workable length of a socket path is what will fit into
+ * struct sockaddr_un. This is usually only 100 or so bytes :-(.
+ *
+ * For consistency, always pass a MAXPGPATH-sized buffer to UNIXSOCK_PATH(),
+ * then complain if the resulting string is = UNIXSOCK_PATH_BUFLEN bytes.
+ * (Because the standard API for getaddrinfo doesn't allow it to complain in
+ * a useful way when the socket pathname is too long, we have to test for
+ * this explicitly, instead of just letting the subroutine return an error.)
+ */
+ #define UNIXSOCK_PATH_BUFLEN sizeof(((struct sockaddr_un *) NULL)-sun_path)
+
+
+ /*
* These manipulate the frontend/backend protocol version number.
*
* The major number should be incremented for incompatible changes. The minor
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9eaf41025beb652c6c242035490d93319a6bc5d0..1386bb791a96fab087af1ba33546ab89772327fd 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** static int
*** 1322,1328
connectDBStart(PGconn *conn)
{
int portnum;
! char portstr[128];
struct addrinfo *addrs = NULL;
struct addrinfo hint;
const char *node;
--- 1322,1328
connectDBStart(PGconn *conn)
{
int portnum;
! char portstr[MAXPGPATH];
struct addrinfo *addrs = NULL;
struct addrinfo hint;
const char *node;
*** connectDBStart(PGconn *conn)
*** 1384,1389
--- 1384,1398
node = NULL;
hint.ai_family = AF_UNIX;
UNIXSOCK_PATH(portstr, portnum, conn-pgunixsocket);
+ if (strlen(portstr) = UNIXSOCK_PATH_BUFLEN)
+ {
+ appendPQExpBuffer(conn-errorMessage,
+ libpq_gettext(Unix-domain socket path \%s\ is too long (maximum %d bytes)\n),
+ portstr,
+ (int) (UNIXSOCK_PATH_BUFLEN - 1));
+ conn-options_valid = false;
+ goto connect_errReturn;
+ }
#else
/* Without Unix sockets, default to localhost instead */
node = DefaultHost;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 07:16 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 06:23 PM, Tom Lane wrote: However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. The test script doesn't do anything. It's pg_upgrade itself that sets the socket dir. See option.c:get_sock_dir() Um ... that's *another* place that needs to change then, because the test script fires up postmasters on its own, outside of pg_upgrade. True, but it doesn't do anything about setting the socket dir, so those just get the compiled-in defaults. What exactly do you want to change about the test script? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 07:16 PM, Tom Lane wrote: Um ... that's *another* place that needs to change then, because the test script fires up postmasters on its own, outside of pg_upgrade. True, but it doesn't do anything about setting the socket dir, so those just get the compiled-in defaults. What exactly do you want to change about the test script? Well, I was thinking about also solving the problem that the compiled-in default might be no good in a build environment. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bugs in CREATE/DROP INDEX CONCURRENTLY
On Fri, Nov 30, 2012 at 4:43 AM, Andres Freund and...@2ndquadrant.comwrote: On 2012-11-29 11:53:50 -0500, Tom Lane wrote: And here is a version for 9.1. This omits the code changes directly relevant to DROP INDEX CONCURRENTLY, but includes the changes to avoid transactional updates of the pg_index row during CREATE CONCURRENTLY, as well as the changes to prevent use of not-valid or not-ready indexes in places where it matters. I also chose to keep on using the IndexIsValid and IndexIsReady macros, so as to avoid unnecessary divergences of the branches. Looks good me. I think this much of the patch needs to go into all supported branches. Looks like that to me, yes. Thanks for all that work! Thanks. Just by looking at the patch it will be necessary to realign the patch of REINDEX CONCURRENTLY. However, as the discussion regarding the lock taken at phase 2 (index swapping) is still not done, I am not sure if it is worth to do that yet. Andres, please let me know in case you want a better version for your review. -- Michael Paquier http://michael.otacoo.com
Re: [HACKERS] missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?
On Fri, Nov 30, 2012 at 3:19 AM, Andres Freund and...@2ndquadrant.comwrote:
Hi,
while looking at trigger.c from the POV of foreign key locks I noticed
that GetTupleForTrigger commentless assumes it can just look at a pages
content without a
LockBuffer(buffer, BUFFER_LOCK_SHARE);
That code path is only reached for AFTER ... FOR EACH ROW ... triggers,
so its fine it's not locking the tuple itself. That already needs to
have happened before.
The code in question:
if (newslot_which_is_passed_by_before_triggers)
...
else
{
Pagepage;
ItemId lp;
buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));
page = BufferGetPage(buffer);
lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
Assert(ItemIdIsNormal(lp));
tuple.t_data = (HeapTupleHeader) PageGetItem(page, lp);
tuple.t_len = ItemIdGetLength(lp);
tuple.t_self = *tid;
tuple.t_tableOid = RelationGetRelid(relation);
}
result = heap_copytuple(tuple);
ReleaseBuffer(buffer);
As can be seen in the excerpt above this is basically a very stripped
down heap_fetch(). But without any locking on the buffer we just read.
I can't believe this is safe? Just about anything but eviction could
happen to that buffer at that moment?
Am I missing something?
That seems to be safe to me. Anything thats been read above can't really
change. The tuple is already locked, so a concurrent update/delete has to
wait on us. We have a pin on the buffer, so VACUUM or HOT-prune can't
happen either. I can't see any other operation that can really change those
fields.
heap_fetch() though looks very similar has an important difference. It
might be reading the tuple without lock on it and we need the buffer lock
to protect against concurrent update/delete to the tuple. AFAIK once the
tuple is passed through qualification checks etc, even callers of
heap_fetch() can read the tuple data without any lock on the buffer itself.
Thanks,
Pavan
--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee
Re: [HACKERS] initdb.c::main() too large
Bruce Momjian br...@momjian.us writes: In looking to add an fsync-only option to initdb, I found its main() function to be 743 lines long, and very hard to understand. The attached patch moves much of that code into separate functions, which will make initdb.c easier to understand, and easier to add an fsync-only option. The original initdb.c author, Andrew Dunstan, has accepted the restructuring, in principle. No objection to breaking it into multiple functions --- but I do say it's a lousy idea to put the long_options[] constant at the front of the file, thousands of lines away from the switch construct that it has to be in sync with. We don't do that in any other program AFAIR. Keep that in the main() function, please. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Patch to fix a crash of psql
1. some especial character
(my sql file contains japanese comment -- コメント . It can cause
psql crash.)
2. PGCLIENTENCODING is SJIS
3. the encoding of input sql file is UTF-8
Actually the problem can occur even when importing following 3 byte
UTF8 input file:
ト
(in hexa, 0xe3, 0x83, 0x88)
In this paticular case, psql decides that the total character length is
5, not 3. Because it just looks at the each first byte by calling PQmblen:
0xe3 - 1 bytes in SJIS
0x83 - 2 bytes in SJIS
0x88 - 2 bytes in SJIS
total: 5 bytes
which is apparently wrong and causes subsequent segfault. Note that it
is possible that input file psql decision case as well if client
encoding is different from file encoding, which will not be good too.
I think we should detect the cases as much as possible and warn users,
rather than silently ignore that fact client encoding != file
encoding. I don't think we can detect it in a reliable way, but at
least we could check the cases above(sum of PQmblen is not equale to
buffer lenghth). So my proposal is, if prepare_buffer() detects
possible inconsistency between buffer encoding and file encoding, warn
user.
[t-ishii@localhost psql]$ PGCLIENTENCODING=SJIS psql postgres
Pager usage is off.
psql (9.3devel)
Type help for help.
postgres=# \i ~/sql
CREATE DATABASE
You are now connected to database mydb as user t-ishii.
CREATE SCHEMA
psql:/home/t-ishii/sql:7: warning: possible conflict between client encoding
SJIS and input file encoding
CREATE TABLE
Comments?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp
diff --git a/src/bin/psql/psqlscan.l b/src/bin/psql/psqlscan.l
index d32a12c..a14d6fe 100644
--- a/src/bin/psql/psqlscan.l
+++ b/src/bin/psql/psqlscan.l
@@ -1808,7 +1808,29 @@ prepare_buffer(const char *txt, int len, char **txtcopy)
newtxt[i] = txt[i];
i++;
while (--thislen 0)
+ {
+if (i = len)
+{
+ /*
+ * This could happen if cur_state-encoding is
+ * different from input file encoding.
+ */
+ psql_error(warning: possible conflict between client encoding %s and input file encoding\n,
+ pg_encoding_to_char(cur_state-encoding));
+ break;
+}
newtxt[i++] = (char) 0xFF;
+ }
+ }
+
+ if (i != len)
+ {
+ /*
+ * This could happen if cur_state-encoding is
+ * different from input file encoding.
+ */
+ psql_error(warning: possible conflict between client encoding %s and input file encoding\n,
+ pg_encoding_to_char(cur_state-encoding));
}
}
--
Sent 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] Patch to fix a crash of psql
buffer lenghth). So my proposal is, if prepare_buffer() detects possible inconsistency between buffer encoding and file encoding, warn user. I agree with that. On 2012/11/30 12:52, Tatsuo Ishii Wrote: 1. some especial character (my sql file contains japanese comment -- コメント . It can cause psql crash.) 2. PGCLIENTENCODING is SJIS 3. the encoding of input sql file is UTF-8 Actually the problem can occur even when importing following 3 byte UTF8 input file: ト (in hexa, 0xe3, 0x83, 0x88) In this paticular case, psql decides that the total character length is 5, not 3. Because it just looks at the each first byte by calling PQmblen: 0xe3 - 1 bytes in SJIS 0x83 - 2 bytes in SJIS 0x88 - 2 bytes in SJIS total: 5 bytes which is apparently wrong and causes subsequent segfault. Note that it is possible that input file psql decision case as well if client encoding is different from file encoding, which will not be good too. I think we should detect the cases as much as possible and warn users, rather than silently ignore that fact client encoding != file encoding. I don't think we can detect it in a reliable way, but at least we could check the cases above(sum of PQmblen is not equale to buffer lenghth). So my proposal is, if prepare_buffer() detects possible inconsistency between buffer encoding and file encoding, warn user. [t-ishii@localhost psql]$ PGCLIENTENCODING=SJIS psql postgres Pager usage is off. psql (9.3devel) Type help for help. postgres=# \i ~/sql CREATE DATABASE You are now connected to database mydb as user t-ishii. CREATE SCHEMA psql:/home/t-ishii/sql:7: warning: possible conflict between client encoding SJIS and input file encoding CREATE TABLE Comments? -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PQconninfo function for libpq
On Wed, Nov 28, 2012 at 7:01 AM, Magnus Hagander mag...@hagander.net wrote: On Nov 28, 2012 1:54 AM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Peter Eisentraut wrote: There is already the PQconninfoOption.dispchar field for this purpose. I had the impression that that field would go away with this patch, but then again it might not be worth the compatibility break. I find the dispchar thingy somewhat unsightly. It is that, without a doubt, but that API has been that way longer than any of us have been working on the code. I'm not excited about changing it just to have an allegedly-cleaner API. And we cannot have the field simply go away, because it's been exposed to applications for lo these many years, and surely some of them are using it. So in practice we'd be maintaining both the old API and the new one. I think we should leave it as-is until there are more reasons to change it than seem to be provided in this thread. I think removing that would be a really bad idea. I'm not sure anybody is actually relying on it, but it would also change the size of the struct and thus break things for anybody using those functions. If people prefer we remove the password classifier for the new function since it at least partially duplicates that field we can certainly do that, but I think leaving it in allows those who write new code to use a slightly neater api, at pretty much no cost in maintenance for us. In the interest of moving things along, I'll remove these for now at least, and commit the rest of the patch, so we can keep working on the basebacku part of things. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
