Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

[Piotr Stefaniak]

 +myextra = (int *) malloc(sizeof(int));

Please consider not casting malloc(). See 
http://c-faq.com/malloc/mallocnocast.html



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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

[Piotr Stefaniak]

On 03/22/2014 04:00 PM, Tom Lane wrote:

That argument is entirely bogus, as it considers only one possible way
in which the call could be wrong; a way that is of very low probability
in PG usage, since we include stdlib.h in our core headers.  Besides
which, as noted in the page itself, most modern compilers would warn
anyway if you forgot the inclusion.

Apart from what the page says, I also think of casting malloc() as bad 
style and felt the need to bring this up. But since you pointed out why 
you don't want to remove the cast, I withdraw my previous suggestion.



On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will *not* help you with.
What if myextra is actually of type int64 *?  In that case you probably
meant to make enough space for an int64 not an int.  But without the cast,
you won't be told you did anything wrong.  This is a particular hazard if
you change your mind later on about the type of myextra.  (A colleague
at Salesforce got burnt in exactly that way, just a couple days ago.)


So perhaps this alternative:
  myextra = malloc(sizeof *myextra);


PS.
Coding style matters to me, but I was and still am far from insisting on 
anything.



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

Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors

[Piotr Stefaniak]

On 03/22/2014 04:00 PM, Tom Lane wrote:

On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will*not*  help you with.
What if myextra is actually of type int64 *?
Indeed, neither gcc -Wall -Wextra -std=c89 -pedantic nor clang 
-Weverything -Wno-shadow -std=c89 -pedantic issues a warning in such 
case. clang --analyze, however, does. Perhaps TenDRA would, if it ever 
worked.


This message is meant to be merely informative, since I've put some 
effort into this test. I'm not trying to argue.
#include stdlib.h

typedef long int int64;

int main(void)
{
  int  *myextra;

  /* with explicit casting */
  myextra = (int *) malloc(sizeof (int));
free(myextra);

  /* with no explicit casting */
  myextra = malloc(sizeof (int));
free(myextra);
  
  /* myextra now becomes int64 */
  {
int64 *myextra;

/* with explicit casting */
myextra = (int *) malloc(sizeof (int)); /* [1], [2]. and [3] warn here */
  free(myextra);

/* with no explicit casting */
myextra = malloc(sizeof (int)); /* Only [3] warns here */
  free(myextra);
  }

  return 0;
}

/*
 1: gcc 4.8.2: gcc -Wall -Wextra -std=c89 -pedantic /tmp/test.c
 2: clang 3.5.0: clang -Weverything -Wno-shadow -std=c89 -pedantic /tmp/test.c
 3: clang 3.5.0: clang --analyze /tmp/test.c
 */

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

[HACKERS] Keepalive-related socket options under FreeBSD 9, 10

[Piotr Stefaniak]

Since upgrading FreeBSD from 8 to 9, I've noticed the following messages
showing up in logs when a connection with pgAdmin3 is made:

LOG:  getsockopt(TCP_KEEPCNT) failed: Protocol not available
STATEMENT:  SELECT setting FROM pg_settings WHERE name IN ('autovacuum',
'track_counts')
LOG:  getsockopt(TCP_KEEPIDLE) failed: Protocol not available
STATEMENT:  SELECT setting FROM pg_settings WHERE name IN ('autovacuum',
'track_counts')
LOG:  getsockopt(TCP_KEEPINTVL) failed: Protocol not available
STATEMENT:  SELECT setting FROM pg_settings WHERE name IN ('autovacuum',
'track_counts')

tcp_keepalives_idle, tcp_keepalives_interval, and tcp_keepalives_count
are all set to the default (0), which means system default.

My guess as to what causes this:

src/backend/libpq/pqcomm.c apparently assumes that if TCP_KEEPIDLE 
friends are defined, then the respective options are readable, but
according to man tcp, that is not the case for FreeBSD 9 (and 10):

TCP_KEEPINIT
This write-only setsockopt(2) option accepts a per-socket
timeout argument of u_int in seconds, for new, non-estab-
lished TCP connections.  For the global default in mil-
liseconds see keepinit in the MIB Variables section fur-
ther down.

As a work-around, I've set the keepalive options to the system defaults
provided by man tcp.


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

Re: [HACKERS] How to use Makefile - pgxs without gcc -O2 ?

[Piotr Stefaniak]

On 07/08/2014 17:53, geohas wrote:
 make CFLAGS=-O0  was it.
 
 now gdb doesn't print Value optimized out.
If you're using GCC 4.8 or later, consider using it with -Og for that
kind of builds.


-- 
Sent 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] Add STRICT to some regression test C functions.

[Piotr Stefaniak]

On 01/09/2016 12:05 AM, Andreas Seltenreich wrote:

Maybe someone can sneak this patch in?


Exactly this is done by a part of another patch, by Michael Paquier, 
that he sent to an off-list thread.


Michael had asked for feedback there, but I didn't have the time to test 
the patch. Just from reading it I think it's good, though. And it still 
applies and passes HEAD's make check-world tests.




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

Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof

[Piotr Stefaniak]

On 2016-05-25 21:13, Tom Lane wrote:

I'd love to see a fix for its brain damage around function pointer typedef 
formatting, too.


Show me a few examples and I'll look into it.


I'm excited about this too, not least because it suggests that maybe bsdindent 
isn't quite as opaque as it appears.


It's old, hacked on many times over the past few decades and 
historically just a band-aid rather than something designed from the 
ground up, so it's not easy to work with. Which is why I think that a 
newer tool (like ClangFormat) should be considered as a replacement for 
pg_bsd_indent.




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

Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof

[Piotr Stefaniak]

On 2016-05-25 21:13, Tom Lane wrote:

Assuming this patch withstands more careful review, we will need to think
about project policy for how/when to apply such fixes.


I discovered yesterday that Bruce Evans had done the fix for sizeof in 
their fork of indent(1) in 2004 (r125623 [1]). The core fix is exactly 
the same as mine, he just did more fixes around it, which I haven't 
analyzed.


I'm trying to see if FreeBSD indent can successfully do pg_bsd_indent's 
job. So far I had to fix one thing, which is not adding a space after a 
cast operator, for which they added no option to turn it off. Currently 
I'm fighting one other bug, but I think that'll be it.


I took interest in FreeBSD's fork of indent(1) because they've fixed 
more things than NetBSD people have in their fork, it seems. I'm also 
hoping it'll be easier to reinvent GNU indent's -tsn ("set tabsize to n 
spaces") option for FreeBSD indent than it would be for any other of the 
forks that aren't GNU. I envision that to be the first step to getting 
rid of some of the work-arounds pgindent does, mainly running entab and 
detab as pre- and post-processing steps.


If porting FreeBSD indent to PostgreSQL's sources turns out to be 
successful, there will be a choice between rebasing pg_bsd_indent on 
that and picking specific patches and applying it on PG's fork of indent(1).


[1] 
https://svnweb.freebsd.org/base/head/usr.bin/indent/lexi.c?r1=125619=125623



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

Re: [HACKERS] A couple of cosmetic changes around shared memory code

[Piotr Stefaniak]

On 2016-05-16 21:40, Piotr Stefaniak wrote:

Hello,

while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.


Revised patch attached.

commit 3ff1afa84e4bc22f153c876e2f0588327a8a004e
Author: Piotr Stefaniak <postg...@piotr-stefaniak.me>
Date:   Thu Apr 28 18:36:16 2016 +0200

Cosmetic improvements around shm_mq and test_shm_mq.

diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index cc1f04d..64a5475 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -103,7 +103,7 @@ struct shm_mq
  * locally by copying the chunks into a backend-local buffer.  mqh_buffer is
  * the buffer, and mqh_buflen is the number of bytes allocated for it.
  *
- * mqh_partial_message_bytes, mqh_expected_bytes, and mqh_length_word_complete
+ * mqh_partial_bytes, mqh_expected_bytes, and mqh_length_word_complete
  * are used to track the state of non-blocking operations.  When the caller
  * attempts a non-blocking operation that returns SHM_MQ_WOULD_BLOCK, they
  * are expected to retry the call at a later time with the same argument;
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 5bd2820..a0f3962 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers,
 	segsize = shm_toc_estimate();
 
 	/* Create the shared memory segment and establish a table of contents. */
-	seg = dsm_create(shm_toc_estimate(), 0);
+	seg = dsm_create(segsize, 0);
 	toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg),
 		 segsize);
 
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index 638649b..a94414a 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -139,7 +139,7 @@ test_shm_mq_main(Datum main_arg)
 	 * we can go ahead and exit.
 	 */
 	dsm_detach(seg);
-	proc_exit(1);
+	proc_exit(0);
 }
 
 /*

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

Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof

[Piotr Stefaniak]

On 2016-05-27 08:13, Piotr Stefaniak wrote:

I'm trying to see if FreeBSD indent can successfully do pg_bsd_indent's
job. So far I had to fix one thing, which is not adding a space after a
cast operator, for which they added no option to turn it off. Currently
I'm fighting one other bug, but I think that'll be it.


So... after fixing 12 times more bugs that I had anticipated (see the 
list at the end of this email; also see attached patches.tgz if you want 
to apply the patches yourself), my "fork" of FreeBSD indent(1) can do 
pg_bsd_indent's job if you pass it three additional parameters (-nut 
-cli1 -sac), producing a 6600-line unified diff, mostly of desired 
changes (see freebsd-indent.diff.gz for details).


I'm in the process of pushing my changes upstream, but I was already 
told that it's too late to get them into 11.0-RELEASE. Personally, I 
don't mind that, hoping that the upstream will accept them eventually.



I'm also hoping it'll be easier to reinvent GNU indent's -tsn ("set
tabsize to n spaces") option for FreeBSD indent than it would be for
any other of the forks that aren't GNU. I envision that to be the
first step to getting rid of some of the work-arounds pgindent does,
mainly running entab and detab as pre- and post-processing steps.


That and more I'll probably do later.


If porting FreeBSD indent to PostgreSQL's sources turns out to be
successful, there will be a choice between rebasing pg_bsd_indent on
that and picking specific patches and applying it on PG's fork of
indent(1).


At this point I think it wouldn't make any sense to port any changes to 
current pg_bsd_indent.



The full list of changes I made to FreeBSD's indent(1) as of r289677:
  [bugfix] Fix typo in keyword "typedef".
  [bugfix] Avoid out of bound access of array codebuf pointed into 
by s_code.

  [bugfix] Avoid out of bound access of array ps.paren_indents.
  [bugfix] Avoid out of bound access of array in_buffer.
  [bugfix] Avoid potential use-after-free.
  [bugfix] Semicolons inside struct declarations don't end the 
declarations.

  [bugfix] Support "f" and "F" floating constant suffixes.
  [bugfix] Removed whitespace shouldn't be considered in column 
calculations.

  [bugfix] Don't add surplus space on empty lines in comments.
  [bugfix] Bail out if there's no more space on the parser stack.
  [bugfix] Consistently indent declarations.
  [bugfix] Don't ignore the fact that offsetof is a keyword.
  [cleanup] Make definition of opchar conditional.
  [cleanup] Remove dead code relating to unix-style comments.
  [cleanup] Simplify pr_comment().
  [cleanup] Deduplicate code that decided if a comment needs a 
blank line at the top.

  [bugfix] Fix wrapping of some lines in comments.
  [bugfix] Untangle the connection between pr_comment.c and io.c, 
fixing at least two bugs

  [feature] Add -sac (space after cast) and -nsac options.
  [bugfix] Don't newline on cpp lines like #endif unless -bacc is on.
  [feature] Add -U option for providing a file containing list of 
types.

  [optimization] Use bsearch() for looking up type keywords.



freebsd-indent.diff.gz
Description: application/gzip


patches.tgz
Description: application/compressed-tar

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

Re: [HACKERS] Releasing in September

[Piotr Stefaniak]

On 01/26/2016 02:09 AM, Peter Eisentraut wrote:
> On 1/25/16 2:48 AM, Torsten Zühlsdorff wrote:

>> In FreeBSD for example there is an online tool for review
>> (http://review.freebsd.org) which was opened to public. There you can
>> review any code, left comments in the parts you wanted, submit different
>> users to it etc.

> I agree better code review tooling could help a bit.  The URL you post
> above doesn't work at the moment (for me), though.

That's a typo, it's reviews.freebsd.org.



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

Re: [HACKERS] Proposal: Trigonometric functions in degrees

[Piotr Stefaniak]

On 01/31/2016 01:23 PM, Michael Paquier wrote:

Per IEEE 754, division by 0 for a double results in Nan or +/-Inf, so
that's actually correct.


I didn't know that. I guess that in practice that is OK and the case is 
closed.


Interestingly to me, that assumption appears to rely on the C 
implementation complying to IEC 60559, in which case C99 lets the 
implementation signal that by defining the __STDC_IEC_559__ macro. C89 
doesn't seem to mention any of this.




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

Re: [HACKERS] Proposal: Trigonometric functions in degrees

[Piotr Stefaniak]

These changes from 65abaab547a5758b0d6d92df4af1663bb47d545f

- result = sign * cosd_q1(arg1) / sind_q1(arg1);
+ result = sign * ((cosd_q1(arg1) / sind_q1(arg1)) / cot_45);

and

- result = sign * sind_q1(arg1) / cosd_q1(arg1);
+ result = sign * ((sind_q1(arg1) / cosd_q1(arg1)) / tan_45);

both introduce division by zero, don't they?



--
Sent 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: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Piotr Stefaniak]

On 2016-04-12 07:00, Andres Freund wrote:

On 2016-04-12 00:32:13 -0400, Tom Lane wrote:

I wrote:

This commit has broken buildfarm member gaur, and no doubt pademelon
will be equally unhappy once it catches up to HEAD.


Spoke too soon, actually: pademelon did not get as far as initdb.

cc: "bufmgr.c", line 4032: error 1521: Incorrect initialization.
cc: "bufmgr.c", line 4058: error 1521: Incorrect initialization.

Apparently, assigning the result of init_spin_delay() is not as portable
as you thought.


Hm. I'm not sure why not though? Is it because delayStatus isn't static
or global scope?


It's because C89 requires initializers for aggregate and union types to 
be constant expressions (3.5.7p3):

All the expressions in an initializer for an object that has static
storage duration or in an initializer list for an object that has
aggregate or union type shall be constant expressions.


C99 removes this constraint (6.7.8p4):

All the expressions in an initializer for an object that has static storage 
duration shall be
constant expressions or string literals.





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

[HACKERS] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c

[Piotr Stefaniak]

Hi,

using sqlsmith and UBSan I have found these two division by zero errors:

src/backend/optimizer/plan/planner.c:4846
/* Convert absolute # of tuples to a fraction; no need to clamp */
if (tuple_fraction >= 1.0)
{
tuple_fraction /= best_path->rows;
}

and

src/backend/optimizer/path/costsize.c:3029
if (subplan->subLinkType == EXISTS_SUBLINK)
{
/* we only need to fetch 1 tuple */
sp_cost.per_tuple += plan_run_cost / plan->plan_rows;
}

The first is triggered by this query (reduced by me from the original 
query string generated by sqlsmith):


select 1
from (
  select ref_0.location as c0
  from public.city as ref_0
) as subq_0
where EXISTS (
  select 1
  from (
select sample_0.collname as c0
from pg_catalog.pg_collation as sample_0
  ) as subq_1
  right join public.tt5 as ref_2
inner join pg_catalog.pg_constraint as ref_4
on (ref_2.z = ref_4.coninhcount )
  on (subq_1.c0 = ref_4.conname ),
  lateral (
select 1
from public.shoelace_candelete as ref_5
where false
  ) as subq_2
);

#0  get_cheapest_fractional_path (rel=0x77ec32a8, tuple_fraction=1) 
at src/backend/optimizer/plan/planner.c:4846
#1  0x007422a1 in make_subplan (root=0xf49778, 
orig_subquery=0x77f593c8, subLinkType=EXISTS_SUBLINK, subLinkId=0, 
testexpr=0x0, isTopQual=1 '\001') at 
src/backend/optimizer/plan/subselect.c:546
#2  0x0074470d in process_sublinks_mutator (node=0x77f610b0, 
context=0x7fffd900) at src/backend/optimizer/plan/subselect.c:1974
#3  0x00744670 in SS_process_sublinks (root=0xf49778, 
expr=0x77f610b0, isQual=1 '\001') at 
src/backend/optimizer/plan/subselect.c:1947
#4  0x00736621 in preprocess_expression (root=0xf49778, 
expr=0x77f610b0, kind=0) at src/backend/optimizer/plan/planner.c:848
#5  0x00736700 in preprocess_qual_conditions (root=0xf49778, 
jtnode=0xf5f790) at src/backend/optimizer/plan/planner.c:893
#6  0x00735ff3 in subquery_planner (glob=0xf3ef70, 
parse=0xf3e9a0, parent_root=0x0, hasRecursion=0 '\000', 
tuple_fraction=0) at src/backend/optimizer/plan/planner.c:600
#7  0x0073566b in standard_planner (parse=0xf3e9a0, 
cursorOptions=256, boundParams=0x0) at 
src/backend/optimizer/plan/planner.c:307
#8  0x007353ad in planner (parse=0xf3e9a0, cursorOptions=256, 
boundParams=0x0) at src/backend/optimizer/plan/planner.c:177
#9  0x00800d3b in pg_plan_query (querytree=0xf3e9a0, 
cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:798
#10 0x00800dee in pg_plan_queries (querytrees=0xf53648, 
cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:857
#11 0x00801093 in exec_simple_query (query_string=0xf07dd8 
"select 1\nfrom (\n  select ref_0.location as c0\n  from public.city as 
ref_0\n) as subq_0\nwhere EXISTS (\n  select 1\n  from (\nselect 
sample_0.collname as c0\nfrom pg_catalog.pg_collation as sample_0\n 
 ) as subq_1\n  right join public.tt5 as ref_2\ninner join 
pg_catalog.pg_constraint as ref_4\non (ref_2.z = ref_4.coninhcount 
)\n  on (subq_1.c0 = ref_4.conname ),\n  lateral (\nselect 1\n 
from public.shoelace_candelete as ref_5\nwhere false\n  ) as 
subq_2\n);") at src/backend/tcop/postgres.c:1022
#12 0x00805355 in PostgresMain (argc=1, argv=0xe95ee0, 
dbname=0xe95d40 "regression", username=0xe95d20 "me") at 
src/backend/tcop/postgres.c:4059
#13 0x0077ed44 in BackendRun (port=0xeb2f80) at 
src/backend/postmaster/postmaster.c:4258
#14 0x0077e4a8 in BackendStartup (port=0xeb2f80) at 
src/backend/postmaster/postmaster.c:3932
#15 0x0077ac2c in ServerLoop () at 
src/backend/postmaster/postmaster.c:1690
#16 0x0077a261 in PostmasterMain (argc=5, argv=0xe94e10) at 
src/backend/postmaster/postmaster.c:1298
#17 0x006c623c in main (argc=5, argv=0xe94e10) at 
src/backend/main/main.c:228


The second one is triggered by this (again, reduced from the original):

select 1
from public.tt5 as subq_0
where EXISTS (
  select 1
  from public.b_star as ref_0
  where false
);

#0  cost_subplan (root=0xf3e718, subplan=0xf42780, plan=0xf3fcd8) at 
src/backend/optimizer/path/costsize.c:3029
#1  0x00742eb9 in build_subplan (root=0xf3e718, plan=0xf3fcd8, 
subroot=0xf3f6a8, plan_params=0x0, subLinkType=EXISTS_SUBLINK, 
subLinkId=0, testexpr=0x0, adjust_testexpr=1 '\001', unknownEqFalse=1 
'\001') at src/backend/optimizer/plan/subselect.c:887
#2  0x007422c0 in make_subplan (root=0xf3e718, 
orig_subquery=0xf09628, subLinkType=EXISTS_SUBLINK, subLinkId=0, 
testexpr=0x0, isTopQual=1 '\001') at 
src/backend/optimizer/plan/subselect.c:551
#3  0x007446d7 in process_sublinks_mutator (node=0xf3f100, 
context=0x7fffd900) at src/backend/optimizer/plan/subselect.c:1974
#4  0x0074463a in SS_process_sublinks (root=0xf3e718, 

Re: [HACKERS] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c

[Piotr Stefaniak]

On 2016-03-26 19:29, Piotr Stefaniak wrote:

I'm not saying this is necessarily a bug since the whole function deals
with floats, but perhaps it's interesting to note that ndistinct can be
0 in src/backend/utils/adt/selfuncs.c:estimate_hash_bucketsize:


On the exact same note, something like this (again reduced from what 
sqlsmith produced):


select 1
from unnest('{}'::boolean[]) a (x)
left join (
  select *
  from unnest('{}'::boolean[])
  where false
) b (x) on a.x = b.x;

leads to vardata.rel->tuples being zero here:
if (vardata.rel)
ndistinct *= vardata.rel->rows / vardata.rel->tuples;

Back-trace attached.

#0  estimate_hash_bucketsize (root=0xf3cf98, hashkey=0xf44398, nbuckets=1024) 
at src/backend/utils/adt/selfuncs.c:3543
#1  0x007141a4 in final_cost_hashjoin (root=0xf3cf98, path=0xf47118, 
workspace=0x7fffd950, sjinfo=0xf45460, semifactors=0x7fffdae8) at 
src/backend/optimizer/path/costsize.c:2856
#2  0x0075c134 in create_hashjoin_path (root=0xf3cf98, 
joinrel=0xf46438, jointype=JOIN_LEFT, workspace=0x7fffd950, 
sjinfo=0xf45460, semifactors=0x7fffdae8, outer_path=0xf461a0, 
inner_path=0xf45a98, restrict_clauses=0xf466f0, required_outer=0x0, 
hashclauses=0xf471d8) at src/backend/optimizer/util/pathnode.c:2133
#3  0x0072074a in try_hashjoin_path (root=0xf3cf98, joinrel=0xf46438, 
outer_path=0xf461a0, inner_path=0xf45a98, hashclauses=0xf471d8, 
jointype=JOIN_LEFT, extra=0x7fffdad0) at 
src/backend/optimizer/path/joinpath.c:523
#4  0x007219e2 in hash_inner_and_outer (root=0xf3cf98, 
joinrel=0xf46438, outerrel=0xf44cd0, innerrel=0xf44fa8, jointype=JOIN_LEFT, 
extra=0x7fffdad0) at src/backend/optimizer/path/joinpath.c:1403
#5  0x00720058 in add_paths_to_joinrel (root=0xf3cf98, 
joinrel=0xf46438, outerrel=0xf44cd0, innerrel=0xf44fa8, jointype=JOIN_LEFT, 
sjinfo=0xf45460, restrictlist=0xf466f0) at 
src/backend/optimizer/path/joinpath.c:211
#6  0x00722e38 in make_join_rel (root=0xf3cf98, rel1=0xf44cd0, 
rel2=0xf44fa8) at src/backend/optimizer/path/joinrels.c:771
#7  0x007222dd in make_rels_by_clause_joins (root=0xf3cf98, 
old_rel=0xf44cd0, other_rels=0xf463b8) at 
src/backend/optimizer/path/joinrels.c:274
#8  0x00721f86 in join_search_one_level (root=0xf3cf98, level=2) at 
src/backend/optimizer/path/joinrels.c:96
#9  0x0070dc4d in standard_join_search (root=0xf3cf98, levels_needed=2, 
initial_rels=0xf46380) at src/backend/optimizer/path/allpaths.c:2124
#10 0x0070dbc6 in make_rel_from_joinlist (root=0xf3cf98, 
joinlist=0xf452c8) at src/backend/optimizer/path/allpaths.c:2055
#11 0x0070b304 in make_one_rel (root=0xf3cf98, joinlist=0xf452c8) at 
src/backend/optimizer/path/allpaths.c:175
#12 0x007352be in query_planner (root=0xf3cf98, tlist=0xf440b8, 
qp_callback=0x739ec7 , qp_extra=0x7fffde10) at 
src/backend/optimizer/plan/planmain.c:246
#13 0x00737d89 in grouping_planner (root=0xf3cf98, inheritance_update=0 
'\000', tuple_fraction=0) at src/backend/optimizer/plan/planner.c:1673
#14 0x00736535 in subquery_planner (glob=0xf3ad88, parse=0xf3a458, 
parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) at 
src/backend/optimizer/plan/planner.c:758
#15 0x00735688 in standard_planner (parse=0xf3a458, cursorOptions=256, 
boundParams=0x0) at src/backend/optimizer/plan/planner.c:307
#16 0x007353ca in planner (parse=0xf3a458, cursorOptions=256, 
boundParams=0x0) at src/backend/optimizer/plan/planner.c:177
#17 0x00800d28 in pg_plan_query (querytree=0xf3a458, cursorOptions=256, 
boundParams=0x0) at src/backend/tcop/postgres.c:798
#18 0x00800ddb in pg_plan_queries (querytrees=0xf3cf60, 
cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:857
#19 0x00801080 in exec_simple_query (query_string=0xf077a8 "select 
1\nfrom unnest('{}'::boolean[]) a (x)\nleft join (\n  select *\n  from 
unnest('{}'::boolean[])\n  where false\n) b (x) on a.x = b.x;") at 
src/backend/tcop/postgres.c:1022
#20 0x00805342 in PostgresMain (argc=1, argv=0xe958d0, dbname=0xe95730 
"postgres", username=0xe95710 "me") at src/backend/tcop/postgres.c:4059
#21 0x0077ed31 in BackendRun (port=0xeb2950) at 
src/backend/postmaster/postmaster.c:4258
#22 0x0077e495 in BackendStartup (port=0xeb2950) at 
src/backend/postmaster/postmaster.c:3932
#23 0x0077ac19 in ServerLoop () at 
src/backend/postmaster/postmaster.c:1690
#24 0x0077a24e in PostmasterMain (argc=3, argv=0xe94850) at 
src/backend/postmaster/postmaster.c:1298
#25 0x006c6216 in main (argc=3, argv=0xe94850) at 
src/backend/main/main.c:228
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c

[Piotr Stefaniak]

I'm not saying this is necessarily a bug since the whole function deals 
with floats, but perhaps it's interesting to note that ndistinct can be 
0 in src/backend/utils/adt/selfuncs.c:estimate_hash_bucketsize:

/*
 * Initial estimate of bucketsize fraction is 1/nbuckets as long as the
 * number of buckets is less than the expected number of distinct 
values;
 * otherwise it is 1/ndistinct.
 */
if (ndistinct > nbuckets)
estfract = 1.0 / nbuckets;
else
estfract = 1.0 / ndistinct;

for this query:
select subq_0.c1 as c0
from (
  select ref_0.a as c0, (select NULL::integer from 
information_schema.user_defined_types limit 1 offset 1) as c1

  from public.rtest_nothn3 as ref_0
  limit 130
) as subq_0
left join (
  select sample_0.x as c0
  from public.insert_tbl as sample_0
  where false
) as subq_1 on subq_0.c1 = subq_1.c0;


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

Re: [HACKERS] AssertArg failure in src/backend/executor/functions.c:check_sql_fn_retval()

[Piotr Stefaniak]

On 2016-03-27 16:40, Tom Lane wrote:

Hm.  I would argue that it should have rejected CAST(NULL AS ANYARRAY).
That's a pseudotype and so there should never be an actual value of that
type, not even a null value.


I'm a little confused about what you mean here. I thought reject was 
exactly what's happening; normally you'd get "ERROR:  return type 
anyarray is not supported for SQL functions".


If you mean specifically to forbid CAST(NULL AS ANYARRAY) in general 
then I'd like to point out that there are columns of type anyarray, at 
least pg_catalog.pg_statistic.stavalues1 is, so the cast is not the only 
way to trigger this.




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

[HACKERS] AssertArg failure in src/backend/executor/functions.c:check_sql_fn_retval()

[Piotr Stefaniak]

Hi,

using sqlsmith I found a way to induce an AssertArg failure in 
src/backend/executor/functions.c:check_sql_fn_retval() for 
assert-enabled builds. It boils down to creating a function and calling 
it like this:


CREATE FUNCTION bad_argument_assert(anyarray, integer) RETURNS anyarray 
LANGUAGE sql AS $$select $1$$;

SELECT bad_argument_assert(CAST(NULL AS ANYARRAY), 0);

TRAP: BadArgument("!(!((rettype) == 2283 || (rettype) == 2277 || 
(rettype) == 2776 || (rettype) == 3500 || (rettype) == 3831))", File: 
"src/backend/executor/functions.c", Line: 1539)


Back-trace attached.

I've done a little bit of git-bisecting and this potential failure is 
there since at least February 2012.


The comment above the function definition specifically says that "we 
should never see a polymorphic pseudotype such as ANYELEMENT as rettype" 
but given that a non-assert-enabled build seems to cope with such 
pseudotypes well, I'm unsure the AssertArg is needed there at all.
#0  0x77341a98 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:55
#1  0x7734369a in __GI_abort () at abort.c:89
#2  0x0094d594 in ExceptionalCondition (conditionName=0xb03c60 
"!(!((rettype) == 2283 || (rettype) == 2277 || (rettype) == 2776 || (rettype) 
== 3500 || (rettype) == 3831))", errorType=0xb03c51 "BadArgument", 
fileName=0xb03890 "src/backend/executor/functions.c", lineNumber=1539) at 
src/backend/utils/error/assert.c:54
#3  0x006840c9 in check_sql_fn_retval (func_id=65539, rettype=2277, 
queryTreeList=0xf3b460, modifyTargetList=0x7fffc676 "", junkFilter=0x0) at 
src/backend/executor/functions.c:1539
#4  0x0075735c in inline_function (funcid=65539, result_type=2277, 
result_collid=0, input_collid=0, args=0xf09dd0, funcvariadic=0 '\000', 
func_tuple=0x77f99c30, context=0x7fffd960) at 
src/backend/optimizer/util/clauses.c:4706
#5  0x00756508 in simplify_function (funcid=65539, result_type=2277, 
result_typmod=-1, result_collid=0, input_collid=0, args_p=0x7fffc838, 
funcvariadic=0 '\000', process_args=1 '\001', allow_non_const=1 '\001', 
context=0x7fffd960) at src/backend/optimizer/util/clauses.c:4187
#6  0x00753f87 in eval_const_expressions_mutator (node=0xf09960, 
context=0x7fffd960) at src/backend/optimizer/util/clauses.c:2834
#7  0x006cbb2f in expression_tree_mutator (node=0xf099b8, 
mutator=0x7539ac , context=0x7fffd960) at 
src/backend/nodes/nodeFuncs.c:2640
#8  0x00755ead in eval_const_expressions_mutator (node=0xf099b8, 
context=0x7fffd960) at src/backend/optimizer/util/clauses.c:3806
#9  0x006cbd2a in expression_tree_mutator (node=0xf09928, 
mutator=0x7539ac , context=0x7fffd960) at 
src/backend/nodes/nodeFuncs.c:2689
#10 0x00755ead in eval_const_expressions_mutator (node=0xf09928, 
context=0x7fffd960) at src/backend/optimizer/util/clauses.c:3806
#11 0x00753959 in eval_const_expressions (root=0xf09ae0, node=0xf09928) 
at src/backend/optimizer/util/clauses.c:2676
#12 0x007365f7 in preprocess_expression (root=0xf09ae0, expr=0xf09928, 
kind=1) at src/backend/optimizer/plan/planner.c:831
#13 0x00735f1c in subquery_planner (glob=0xf093a0, parse=0xf09048, 
parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) at 
src/backend/optimizer/plan/planner.c:581
#14 0x00735688 in standard_planner (parse=0xf09048, cursorOptions=256, 
boundParams=0x0) at src/backend/optimizer/plan/planner.c:307
#15 0x007353ca in planner (parse=0xf09048, cursorOptions=256, 
boundParams=0x0) at src/backend/optimizer/plan/planner.c:177
#16 0x00800d28 in pg_plan_query (querytree=0xf09048, cursorOptions=256, 
boundParams=0x0) at src/backend/tcop/postgres.c:798
#17 0x00800ddb in pg_plan_queries (querytrees=0xf09aa8, 
cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:857
#18 0x00801080 in exec_simple_query (query_string=0xf07ee8 "select 
public.bad_argument_assert(CAST(NULL AS ANYARRAY), 0);") at 
src/backend/tcop/postgres.c:1022
#19 0x00805342 in PostgresMain (argc=1, argv=0xe95e90, dbname=0xe95cf0 
"regression", username=0xe95cd0 "me") at src/backend/tcop/postgres.c:4059
#20 0x0077ed31 in BackendRun (port=0xeb6290) at 
src/backend/postmaster/postmaster.c:4258
#21 0x0077e495 in BackendStartup (port=0xeb6290) at 
src/backend/postmaster/postmaster.c:3932
#22 0x0077ac19 in ServerLoop () at 
src/backend/postmaster/postmaster.c:1690
#23 0x0077a24e in PostmasterMain (argc=3, argv=0xe94e10) at 
src/backend/postmaster/postmaster.c:1298
#24 0x006c6216 in main (argc=3, argv=0xe94e10) at 
src/backend/main/main.c:228

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

[HACKERS] Small fix: avoid passing null pointers to memcpy()

[Piotr Stefaniak]

Hello,

from running the regression test suite (including TAP tests) and also 
sqlsmith, I've got a couple of places where UBSan reported calls to 
memcpy() with null pointer passed as either source or destination.


Patch attached.
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index cfb3b50..600824a 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
 		memset(ivbuf, 0, ivs);
 		if (ivlen > ivs)
 			memcpy(ivbuf, iv, ivs);
-		else
+		else if (ivlen > 0)
 			memcpy(ivbuf, iv, ivlen);
 	}
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 34ba385..67c7586 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
 	/*
 	 * copy the scan key, if appropriate
 	 */
-	if (key != NULL)
+	if (key != NULL && scan->rs_nkeys > 0)
 		memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData));
 
 	/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 7e37331..e7599be 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4872,8 +4872,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
 	{
 		if (TransactionIdIsValid(s->transactionId))
 			workspace[i++] = s->transactionId;
-		memcpy([i], s->childXids,
-			   s->nChildXids * sizeof(TransactionId));
+		if (s->nChildXids > 0)
+			memcpy([i], s->childXids,
+   s->nChildXids * sizeof(TransactionId));
 		i += s->nChildXids;
 	}
 	Assert(i == nxids);
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b88e012..dc7a323 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -402,12 +402,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
 	CurrentSnapshot->xmax = sourcesnap->xmax;
 	CurrentSnapshot->xcnt = sourcesnap->xcnt;
 	Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
-	memcpy(CurrentSnapshot->xip, sourcesnap->xip,
-		   sourcesnap->xcnt * sizeof(TransactionId));
+	if (sourcesnap->xcnt > 0)
+		memcpy(CurrentSnapshot->xip, sourcesnap->xip,
+			   sourcesnap->xcnt * sizeof(TransactionId));
 	CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
 	Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
-	memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
-		   sourcesnap->subxcnt * sizeof(TransactionId));
+	if (sourcesnap->subxcnt > 0)
+		memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
+			   sourcesnap->subxcnt * sizeof(TransactionId));
 	CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
 	CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
 	/* NB: curcid should NOT be copied, it's a local matter */

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

Re: [HACKERS] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c

[Piotr Stefaniak]

On 2016-03-28 11:33, Aleksander Alekseev wrote:

Hello, Piotr.

Thanks for report. But I'm having some difficulties reproducing issues
you described.


Oh, if you want backtraces then either set a conditional breakpoint or 
add your own Assert like I did.



Also it would be a good idea to include these steps to regression tests.


I agree and I generally think that the more test cases touch previously 
not covered code paths the better, even if it had to be run as a 
different make(1) target. Although it seems that at least some people 
would agree (see [1]), the "make check" split somehow isn't happening.


[1] ca+tgmoymofe94+3wg3spg9sqajkv4sixjey+rdrmamye-vq...@mail.gmail.com



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

Re: [HACKERS] A couple of cosmetic changes around shared memory code

[Piotr Stefaniak]

On 2016-05-17 19:05, Tom Lane wrote:

Michael Paquier <michael.paqu...@gmail.com> writes:

On Tue, May 17, 2016 at 4:40 AM, Piotr Stefaniak
<postg...@piotr-stefaniak.me> wrote:
-toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry)
+toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry)
 + allocated_bytes;



I don't recall the exact reason, but this is intentional style
(memories from a patchwork with Tom).


Well, it's not so much intentional as that pgindent will make it look like
that no matter what you do --- it's got some weird interaction with
sizeof, offsetof, and typedef names versus operators later on the same
line.  I'd call that a pgindent bug myself, but have no particular desire
to try to fix it.


From my understanding, it's because pg_bsd_indent thinks that 
"(shm_toc)" is a cast since shm_toc is a keyword (type alias fetched 
from the "list of typedefs" file in this case, to be precise), forcing 
the "+" to be a unary operator instead of binary.


One easy way to work around this bug is putting shm_toc into its own 
parentheses but I'd prefer dropping this particular fix from my 
"cosmetic changes" patch until pg_bsd_indent is fixed.


I may come up with a possible fix for this bug, but don't hold your breath.


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

[HACKERS] pg_bsd_indent - improvements around offsetof and sizeof

[Piotr Stefaniak]

Hello,

I think I've managed to improve pg_bsd_indent's handling of two types of 
cases.


The first are like in this example:
-   hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) +1);
+   hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) + 1);
Pristine pg_bsd_indent is inconsistent in masking parentheses as those 
that are part of a cast and those that "are part of sizeof": seeing a 
type name following an lparen it always masks that lparen as a part of a 
cast; seeing an rparen it only removes the bit if it doesn't overlap 
with sizeof_mask. In the example above, "(HTAB" started both "cast 
parens" and "sizeof parens" at the same time, and the immediately 
following rparen ended only the "sizeof parens". According to indent, 
the cast-to type then ends at "tabname)" and what follows is the cast's 
operand, including the + operator; in that case it's assumed to be unary 
and not binary, which is why indent doesn't add the space after it.

The fix was to make it consistent about masking parens:
-   ps.cast_mask |= 1 << ps.p_l_follow;
+   ps.cast_mask |= (1 << ps.p_l_follow & 
~ps.sizeof_mask);

The second type of cases are like this:
-   nse = palloc(offsetof(PLpgSQL_nsitem, name) +strlen(name) + 1);
+   nse = palloc(offsetof(PLpgSQL_nsitem, name) + strlen(name) + 1);
pg_bsd_indent simply hasn't been taught that a parenthesized type name 
following the offsetof macro and then an lparen is another exception to 
the rule of thumb that a construction like that generally means a cast.


You'll also notice other, seemingly unrelated changes, most notably the 
rearrangement in numbers assigned to keywords. I've done it that way so 
that it was easier and simpler to keep the -bs option functioning as 
designed.


I've also renamed "sizeof_mask" to "not_cast_mask", because I think the 
latter is a better description of what the mask does (it prevents 
interpreting parenthesized type names as a cast where they aren't, 
namely where they follow sizeof or offsetof; I haven't done any support 
for function declarators and I don't plan to - the fact that 
pg_bsd_indent thinks that "(int" in "char func(int);" begins a cast is 
amusing but it seems harmless for now).


I'm attaching the patch for pg_bsd_indent and also a full diff that 
shows the change in its behavior when run against PG's sources.
diff -Burw indent.c indent.c
--- indent.c	2014-01-31 04:06:43.0 +0100
+++ indent.c	2016-05-22 19:24:01.666077311 +0200
@@ -568,7 +568,9 @@
 		 * happy */
 			if (ps.want_blank && *token != '[' &&
 			(ps.last_token != ident || proc_calls_space
-|| (ps.its_a_keyword && (!ps.sizeof_keyword || Bill_Shannon
+			/* offsetof (1) is never allowed a space; sizeof (2) iff -bs;
+			 * all other keywords (>2) always get a space before lparen */
+|| (ps.keyword + Bill_Shannon > 2)))
 *e_code++ = ' ';
 			if (ps.in_decl && !ps.block_init) {
 if (troff && !ps.dumped_decl_indent && !is_procname && ps.last_token == decl) {
@@ -601,17 +603,19 @@
 			 * structure decl or
 			 * initialization */
 			}
-			if (ps.sizeof_keyword)
-ps.sizeof_mask |= 1 << ps.p_l_follow;
+			/* a parenthesized type name following sizeof or offsetof is not
+			 * a cast */
+			if (ps.keyword == 1 || ps.keyword == 2)
+ps.not_cast_mask |= 1 << ps.p_l_follow;
 			break;
 
 		case rparen:	/* got a ')' or ']' */
 			rparen_count--;
-			if (ps.cast_mask & (1 << ps.p_l_follow) & ~ps.sizeof_mask) {
+			if (ps.cast_mask & (1 << ps.p_l_follow) & ~ps.not_cast_mask) {
 ps.last_u_d = true;
 ps.cast_mask &= (1 << ps.p_l_follow) - 1;
 			}
-			ps.sizeof_mask &= (1 << ps.p_l_follow) - 1;
+			ps.not_cast_mask &= (1 << ps.p_l_follow) - 1;
 			if (--ps.p_l_follow < 0) {
 ps.p_l_follow = 0;
 diag(0, "Extra %c", *token);
@@ -780,7 +784,7 @@
 			if (ps.last_token == rparen && rparen_count == 0)
 ps.in_parameter_declaration = 0;
 			ps.cast_mask = 0;
-			ps.sizeof_mask = 0;
+			ps.not_cast_mask = 0;
 			ps.block_init = 0;
 			ps.block_init_level = 0;
 			ps.just_saw_decl--;
@@ -1042,7 +1046,7 @@
 	copy_id:
 			if (ps.want_blank)
 *e_code++ = ' ';
-			if (troff && ps.its_a_keyword) {
+			if (troff && ps.keyword) {
 e_code = chfont(, , e_code);
 for (t_ptr = token; *t_ptr; ++t_ptr) {
 	CHECK_SIZE_CODE;
diff -Burw indent_globs.h indent_globs.h
--- indent_globs.h	2005-11-15 01:30:24.0 +0100
+++ indent_globs.h	2016-05-22 19:23:45.067093287 +0200
@@ -255,10 +255,10 @@
  * comment. In that case, the first non-blank
  * char should be lined up with the comment / */
 	int comment_delta, n_comment_delta;
-	int cast_mask;	/* indicates which close parens close off
- * casts */
-	int sizeof_mask;	/* indicates which close parens close off
- * sizeof''s */
+	int cast_mask;	/* indicates which close parens potentially
+ * close off casts */
+	int 

[HACKERS] A couple of cosmetic changes around shared memory code

[Piotr Stefaniak]

Hello,

while investigating the shm_mq code and its testing module I made some 
cosmetic improvements there. You can see them in the attached diff file.
commit 0e202cb6e0eca2e7fb3e1353b550f3d2ace9680e
Author: Piotr Stefaniak <postg...@piotr-stefaniak.me>
Date:   Thu Apr 28 18:36:16 2016 +0200

Cosmetic improvements around shm_mq and test_shm_mq.

diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 7859f42..292d515 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -103,7 +103,7 @@ struct shm_mq
  * locally by copying the chunks into a backend-local buffer.  mqh_buffer is
  * the buffer, and mqh_buflen is the number of bytes allocated for it.
  *
- * mqh_partial_message_bytes, mqh_expected_bytes, and mqh_length_word_complete
+ * mqh_partial_bytes, mqh_expected_bytes, and mqh_length_word_complete
  * are used to track the state of non-blocking operations.  When the caller
  * attempts a non-blocking operation that returns SHM_MQ_WOULD_BLOCK, they
  * are expected to retry the call at a later time with the same argument;
diff --git a/src/backend/storage/ipc/shm_toc.c b/src/backend/storage/ipc/shm_toc.c
index 55248c2..e1d6bd1 100644
--- a/src/backend/storage/ipc/shm_toc.c
+++ b/src/backend/storage/ipc/shm_toc.c
@@ -96,7 +96,7 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
 	total_bytes = vtoc->toc_total_bytes;
 	allocated_bytes = vtoc->toc_allocated_bytes;
 	nentry = vtoc->toc_nentry;
-	toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry)
+	toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry)
 		+ allocated_bytes;
 
 	/* Check for memory exhaustion and overflow. */
@@ -132,7 +132,7 @@ shm_toc_freespace(shm_toc *toc)
 	nentry = vtoc->toc_nentry;
 	SpinLockRelease(>toc_mutex);
 
-	toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry);
+	toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry);
 	Assert(allocated_bytes + BUFFERALIGN(toc_bytes) <= total_bytes);
 	return total_bytes - (allocated_bytes + BUFFERALIGN(toc_bytes));
 }
@@ -176,7 +176,7 @@ shm_toc_insert(shm_toc *toc, uint64 key, void *address)
 	total_bytes = vtoc->toc_total_bytes;
 	allocated_bytes = vtoc->toc_allocated_bytes;
 	nentry = vtoc->toc_nentry;
-	toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry)
+	toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry)
 		+ allocated_bytes;
 
 	/* Check for memory exhaustion and overflow. */
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 5bd2820..a0f3962 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers,
 	segsize = shm_toc_estimate();
 
 	/* Create the shared memory segment and establish a table of contents. */
-	seg = dsm_create(shm_toc_estimate(), 0);
+	seg = dsm_create(segsize, 0);
 	toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg),
 		 segsize);
 
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index 638649b..a94414a 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -139,7 +139,7 @@ test_shm_mq_main(Datum main_arg)
 	 * we can go ahead and exit.
 	 */
 	dsm_detach(seg);
-	proc_exit(1);
+	proc_exit(0);
 }
 
 /*

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

Re: [HACKERS] Small fix: avoid passing null pointers to memcpy()

[Piotr Stefaniak]

Added a fix for src/backend/storage/ipc/shm_mq.c:shm_mq_receive.
commit 936c7268b61460deb201b9e6bbfb60ab5258ec87
Author: Piotr Stefaniak <postg...@piotr-stefaniak.me>
Date:   Thu Apr 28 18:35:43 2016 +0200

Don't pass null pointers to functions that require the pointers to be non null.

diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index cfb3b50..600824a 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
 		memset(ivbuf, 0, ivs);
 		if (ivlen > ivs)
 			memcpy(ivbuf, iv, ivs);
-		else
+		else if (ivlen > 0)
 			memcpy(ivbuf, iv, ivlen);
 	}
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 950bfc8..bf9a7ab 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
 	/*
 	 * copy the scan key, if appropriate
 	 */
-	if (key != NULL)
+	if (key != NULL && scan->rs_nkeys > 0)
 		memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData));
 
 	/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 95690ff..4ae98e6 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4890,8 +4890,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
 	{
 		if (TransactionIdIsValid(s->transactionId))
 			workspace[i++] = s->transactionId;
-		memcpy([i], s->childXids,
-			   s->nChildXids * sizeof(TransactionId));
+		if (s->nChildXids > 0)
+			memcpy([i], s->childXids,
+   s->nChildXids * sizeof(TransactionId));
 		i += s->nChildXids;
 	}
 	Assert(i == nxids);
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index b4dc617..a72795c 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1563,9 +1563,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
 
 	/* copy running xacts */
 	sz = sizeof(TransactionId) * builder->running.xcnt_space;
-	memcpy(ondisk_c, builder->running.xip, sz);
-	COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
-	ondisk_c += sz;
+	if (sz > 0)
+	{
+		memcpy(ondisk_c, builder->running.xip, sz);
+		COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
+		ondisk_c += sz;
+	}
 
 	/* copy committed xacts */
 	sz = sizeof(TransactionId) * builder->committed.xcnt;
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 7d1c9cd..7859f42 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -492,7 +492,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
 	shm_mq_result res;
 	Size		rb = 0;
 	Size		nbytes;
-	void	   *rawdata;
+	void	   *rawdata = NULL;
 
 	Assert(mq->mq_receiver == MyProc);
 
@@ -673,7 +673,11 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
 
 		/* Copy as much as we can. */
 		Assert(mqh->mqh_partial_bytes + rb <= nbytes);
-		memcpy(>mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
+		if (rb > 0)
+		{
+			Assert(rawdata != NULL);
+			memcpy(>mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
+		}
 		mqh->mqh_partial_bytes += rb;
 
 		/*
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 0a9a231..ff09171 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -506,12 +506,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
 	CurrentSnapshot->xmax = sourcesnap->xmax;
 	CurrentSnapshot->xcnt = sourcesnap->xcnt;
 	Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
-	memcpy(CurrentSnapshot->xip, sourcesnap->xip,
-		   sourcesnap->xcnt * sizeof(TransactionId));
+	if (sourcesnap->xcnt > 0)
+		memcpy(CurrentSnapshot->xip, sourcesnap->xip,
+			   sourcesnap->xcnt * sizeof(TransactionId));
 	CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
 	Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
-	memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
-		   sourcesnap->subxcnt * sizeof(TransactionId));
+	if (sourcesnap->subxcnt > 0)
+		memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
+			   sourcesnap->subxcnt * sizeof(TransactionId));
 	CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
 	CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
 	/* NB: curcid should NOT be copied, it's a local matter */
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 0e815a9..0f26bd8 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -1639,6 +1639,7 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
 static bool
 TransactionIdInArray(TransactionId xid, TransactionId

Re: [HACKERS] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Piotr Stefaniak]

On 2016-05-05 09:32, Fabien COELHO wrote:

I note that C99 specifically mentions this as something a compiler
might warn about: [...]


Indeed. Neither gcc nor clang emit such warnings... but they might some
day, which would be a blow for my suggestion!


For what it's worth, newer versions of clang can emit useful warnings 
for cases like this:


int main(void) {
enum test { FOUR = 4 };
enum incompatible { INCOMPATIBLE_FOUR = 4 };
enum test variable;
variable = INCOMPATIBLE_FOUR;
variable = 5;
variable = 4;
variable = 3;

return 0;
}

enum.c:5:13: warning: implicit conversion from enumeration type 'enum 
incompatible' to different enumeration type 'enum test' [-Wenum-conversion]

variable = INCOMPATIBLE_FOUR;
 ~ ^
enum.c:6:13: warning: integer constant not in range of enumerated type 
'enum test' [-Wassign-enum]

variable = 5;
   ^
enum.c:8:13: warning: integer constant not in range of enumerated type 
'enum test' [-Wassign-enum]

variable = 3;
   ^
3 warnings generated.

So with -Wenum-conversion -Wassign-enum you could treat enum types as 
distinct and incompatible with each other.




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

Re: [HACKERS] Small fix: avoid passing null pointers to memcpy()

[Piotr Stefaniak]

On 2016-04-03 09:24, Piotr Stefaniak wrote:

from running the regression test suite (including TAP tests) and also
sqlsmith, I've got a couple of places where UBSan reported calls to
memcpy() with null pointer passed as either source or destination.

Patch attached.


Patch updated.

Since this time the patch includes fixes for other standard library 
function calls (memset and bsearch), I'm renaming the patch file to be 
more generic.


commit 75e849e83c8f7d6b4caab13271b7b0fcf124d497
Author: Piotr Stefaniak <postg...@piotr-stefaniak.me>
Date:   Sat Apr 16 14:28:34 2016 +0200

Don't pass null pointers to functions that require the pointers to be non null.

diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index cfb3b50..600824a 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
 		memset(ivbuf, 0, ivs);
 		if (ivlen > ivs)
 			memcpy(ivbuf, iv, ivs);
-		else
+		else if (ivlen > 0)
 			memcpy(ivbuf, iv, ivlen);
 	}
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 29fd31a..2ed56ab 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
 	/*
 	 * copy the scan key, if appropriate
 	 */
-	if (key != NULL)
+	if (key != NULL && scan->rs_nkeys > 0)
 		memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData));
 
 	/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 7e37331..e7599be 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4872,8 +4872,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
 	{
 		if (TransactionIdIsValid(s->transactionId))
 			workspace[i++] = s->transactionId;
-		memcpy([i], s->childXids,
-			   s->nChildXids * sizeof(TransactionId));
+		if (s->nChildXids > 0)
+			memcpy([i], s->childXids,
+   s->nChildXids * sizeof(TransactionId));
 		i += s->nChildXids;
 	}
 	Assert(i == nxids);
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index b4dc617..a72795c 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1563,9 +1563,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
 
 	/* copy running xacts */
 	sz = sizeof(TransactionId) * builder->running.xcnt_space;
-	memcpy(ondisk_c, builder->running.xip, sz);
-	COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
-	ondisk_c += sz;
+	if (sz > 0)
+	{
+		memcpy(ondisk_c, builder->running.xip, sz);
+		COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
+		ondisk_c += sz;
+	}
 
 	/* copy committed xacts */
 	sz = sizeof(TransactionId) * builder->committed.xcnt;
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 8aa1f49..25ac26f 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -506,12 +506,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
 	CurrentSnapshot->xmax = sourcesnap->xmax;
 	CurrentSnapshot->xcnt = sourcesnap->xcnt;
 	Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount());
-	memcpy(CurrentSnapshot->xip, sourcesnap->xip,
-		   sourcesnap->xcnt * sizeof(TransactionId));
+	if (sourcesnap->xcnt > 0)
+		memcpy(CurrentSnapshot->xip, sourcesnap->xip,
+			   sourcesnap->xcnt * sizeof(TransactionId));
 	CurrentSnapshot->subxcnt = sourcesnap->subxcnt;
 	Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount());
-	memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
-		   sourcesnap->subxcnt * sizeof(TransactionId));
+	if (sourcesnap->subxcnt > 0)
+		memcpy(CurrentSnapshot->subxip, sourcesnap->subxip,
+			   sourcesnap->subxcnt * sizeof(TransactionId));
 	CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed;
 	CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
 	/* NB: curcid should NOT be copied, it's a local matter */
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 0e815a9..0f26bd8 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -1639,6 +1639,7 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
 static bool
 TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
 {
+	Assert(xip != NULL);
 	return bsearch(, xip, num,
    sizeof(TransactionId), xidComparator) != NULL;
 }
@@ -1675,7 +1676,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
 		return false;
 	}
 	/* check if it's one of our txids, toplevel is also in there */
-	else if (TransactionIdInArray(xmin, snapshot->subxip, snapshot->subxcnt))
+	else if (snapshot->subxcnt > 0 && TransactionIdInArray(xmin, snapshot->sub

Re: [HACKERS] More parallel-query fun

[Piotr Stefaniak]

On 2016-06-16 20:14, Tom Lane wrote:

As of HEAD you can exercise quite a lot of parallel query behavior
by running the regression tests with these settings applied:

force_parallel_mode = regress
max_parallel_workers_per_gather = 2-- this is default at the moment
min_parallel_relation_size = 0
parallel_setup_cost = 0
parallel_tuple_cost = 0

This results in multiple interesting failures, including a core dump



I saw another previously-unreported problem before getting to the crash:



Haven't tried to trace that one down yet.


As I expected, I'm unable to reproduce anything of the above - please 
correct me if I'm wrong, but it all seems to have been fixed.




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

Re: [HACKERS] A couple of cosmetic changes around shared memory code

[Piotr Stefaniak]

On 2016-06-29 18:58, Robert Haas wrote:

This code predates be7558162acc5578d0b2cf0c8d4c76b6076ce352, prior to
which proc_exit(0) forced an immediate, unconditional restart.  It's
true that, given that commit, changing this code to do proc_exit(0)
instead of proc_exit(1) would be harmless.  However, people writing
background workers that might need to work with 9.3 would be best
advised to stick with proc_exit(1).  Therefore, I maintain that this
is not broken and doesn't need to be fixed.


Then I'd argue that it ought to be documented in form of a C comment for 
people writing background workers and for those who might want to "fix" 
this in the future.




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

Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof

[Piotr Stefaniak]

On 2016-05-25 21:13, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> On Sun, May 22, 2016 at 4:16 PM, Piotr Stefaniak
>> <postg...@piotr-stefaniak.me> wrote:
>>> I think I've managed to improve pg_bsd_indent's handling of two types of
>>> cases.
>
>> Wow, that seems pretty great.  I haven't scrutinized your changes to
>> pg_bsd_indent, but effect_on_pg.diff looks like a large improvement.

> Assuming this patch withstands more careful review, we will need to think
> about project policy for how/when to apply such fixes.

The patches have got committed upstream and work well for Postgres. You 
can take FreeBSD indent(1) as of SVN r303746, apply patches from 
https://github.com/pstef/freebsd_indent/commits/pass2 (subject to heavy 
rebasing) and use as pg_bsd_indent for pgindent.

There are more fixes I intend to do, of which the most relevant for 
Postgres are:
1) fixing "function pointer typedef formatting"
2) adding a -tsn option like in GNU indent, for setting how many columns 
a tab character will produce. I had a preliminary patch implementing 
that and I have to say that while it removes the need for entab, it also 
introduces a lot of seemingly pointless changes in formatting which will 
be arguably improvements or regressions.



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

Re: [HACKERS] [GENERAL] C++ port of Postgres

[Piotr Stefaniak]

On 2016-08-16 18:33, Robert Haas wrote:
> It wouldn't be that much work to maintain, either: we'd
> just set up some buildfarm members that compiled using C++ and when
> they turned red, we'd go fix it.

I think that there exist subtle differences between C and C++ that 
without compile-time diagnostic could potentially lead to different 
run-time behavior. As an artificial example:

$ cat ./test.c
#include 

int main(void) {
FILE *f = fopen("test.bin", "w");
if (f == NULL)
return 1;
fwrite("1", sizeof '1', 1, f);
fclose(f);
return 0;
}
$ clang ./test.c -o test
$ ./test
$ hexdump test.bin
000 0031 
004
$ clang++ ./test.c -o test
clang-3.9: warning: treating 'c' input as 'c++' when in C++ mode, this 
behavior is deprecated
$ ./test
$ hexdump test.bin
000 0031
001

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

Re: [HACKERS] Possible spelling fixes

[Piotr Stefaniak]

On 2017-02-06 10:40, Heikki Linnakangas wrote:
> On 02/06/2017 04:50 AM, Josh Soref wrote:
>> NUL-terminated -> NULL-terminated
> 
> When we're talking about NUL-terminated strings, NUL refers to the NUL
> ASCII character. NULL usually refers to a NULL pointer. We're probably
> not consistent about this, but in this context, NUL-terminated isn't
> wrong, so let's leave them as they are.

The C standard talks about how "a byte with all bits set to 0, called
the null character" is used to "terminate a character string"; it
mentions '\0' as "commonly used to represent the null character"; and it
also talks about when snprintf() produces "null-terminated output".

It never mentions ASCII in this context; quite intentionally it avoids
assuming ASCII at all, so that a standard-compliant C implementation may
co-exist with other encodings (like EBCDIC).



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

[HACKERS] pg_bsd_indent: implement -lps ("leave preprocessor space")

[Piotr Stefaniak]

Hello,

this is a patch that Andres asked me for. It makes pg_bsd_indent leave
preprocessor space alone, as in this example:

#if 0
#  if 0
#   if 0
# error
#   endif
#  endif
#else
#  line 7
#endif
diff -ur pg_bsd_indent/args.c pg_bsd_indent_patched/args.c
--- pg_bsd_indent/args.c	2014-01-31 04:09:31.0 +0100
+++ pg_bsd_indent_patched/args.c	2017-02-08 01:59:01.921080544 +0100
@@ -221,6 +221,9 @@
 		"lc", PRO_INT, 0, 0, _comment_max_col
 	},
 	{
+		"lps", PRO_BOOL, false, ON, _preprocessor_space
+	},
+	{
 		"lp", PRO_BOOL, true, ON, _to_parens
 	},
 	{
@@ -269,6 +272,9 @@
 		"nip", PRO_BOOL, true, OFF, _parameters
 	},
 	{
+		"nlps", PRO_BOOL, false, OFF, _preprocessor_space
+	},
+	{
 		"nlp", PRO_BOOL, true, OFF, _to_parens
 	},
 	{
diff -ur pg_bsd_indent/indent.c pg_bsd_indent_patched/indent.c
--- pg_bsd_indent/indent.c	2014-01-31 04:06:43.0 +0100
+++ pg_bsd_indent_patched/indent.c	2017-02-08 01:56:59.039931984 +0100
@@ -1091,17 +1091,25 @@
 			(s_code != e_code))
 dump_line();
 			*e_lab++ = '#';	/* move whole line to 'label' buffer */
+			if (leave_preprocessor_space) {
+while (isblank((int)*buf_ptr)) {
+	CHECK_SIZE_LAB;
+	*e_lab++ = *buf_ptr++;
+}
+			}
+			else {
+while (isblank((int)*buf_ptr)) {
+	buf_ptr++;
+}
+			}
+			t_ptr = e_lab;
+
 			{
 int in_comment = 0;
 int com_start = 0;
 charquote = 0;
 int com_end = 0;
 
-while (*buf_ptr == ' ' || *buf_ptr == '\t') {
-	buf_ptr++;
-	if (buf_ptr >= buf_end)
-		fill_buffer();
-}
 while (*buf_ptr != '\n' || in_comment) {
 	CHECK_SIZE_LAB;
 	*e_lab = *buf_ptr++;
@@ -1179,7 +1187,7 @@
 ps.pcase = false;
 			}
 
-			if (strncmp(s_lab, "#if", 3) == 0) {
+			if (t_ptr[0] == 'i' && t_ptr[1] == 'f') {
 if (blanklines_around_conditional_compilation) {
 	int c;
 	prefix_blankline_requested++;
@@ -1192,7 +1200,7 @@
 } else
 	diag(1, "#if stack overflow");
 			} else
-if (strncmp(s_lab, "#else", 5) == 0) {
+if (t_ptr[0] == 'e' && t_ptr[1] == 'l') {
 	if (ifdef_level <= 0)
 		diag(1, "Unmatched #else");
 	else {
@@ -1200,7 +1208,7 @@
 		ps = state_stack[ifdef_level - 1];
 	}
 } else
-	if (strncmp(s_lab, "#endif", 6) == 0) {
+	if (strncmp(t_ptr, "endif", 5) == 0) {
 		if (ifdef_level <= 0)
 			diag(1, "Unmatched #endif");
 		else {
diff -ur pg_bsd_indent/indent_globs.h pg_bsd_indent_patched/indent_globs.h
--- pg_bsd_indent/indent_globs.h	2005-11-15 01:30:24.0 +0100
+++ pg_bsd_indent_patched/indent_globs.h	2017-02-08 01:57:51.003994806 +0100
@@ -222,6 +222,7 @@
 	 * "for(e;e;e)" should be indented an extra
 	 * tab stop so that they don't conflict with
 	 * the code that follows */
+EXTERN int leave_preprocessor_space;
 
 /* -troff font state information */
 

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

Re: [HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Piotr Stefaniak]

On 2016-08-19 23:55, Tom Lane wrote:
> I think you are failing to understand Heikki's point.  There is no way
> we are committing the change depicted above, because (1) it will mask more
> bugs than it fixes; (2) it's an enormously expensive way to fix anything;
> and (3) it will effectively disable valgrind testing for missed
> initializations.

I wasn't disagreeing with Heikki, I was trying to show that while 
Aleksander's patch may be useless and perhaps harmful if committed, it 
is not useless in a larger perspective as it has made people look into 
an issue.

And I did that simply because I have more changes of that kind that may 
end up being deemed as useless for committing, but I want to share them 
with -hackers anyway.



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

Re: [HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Piotr Stefaniak]

On 2016-08-18 20:02, Heikki Linnakangas wrote:
> On 03/22/2016 03:27 PM, Aleksander Alekseev wrote:
>> diff --git a/src/backend/utils/mmgr/aset.c
>> b/src/backend/utils/mmgr/aset.c
>> index d26991e..46ab8a2 100644
>> --- a/src/backend/utils/mmgr/aset.c
>> +++ b/src/backend/utils/mmgr/aset.c
>> @@ -850,7 +850,7 @@ AllocSetAlloc(MemoryContext context, Size size)
>>  blksize <<= 1;
>>
>>  /* Try to allocate it */
>> -block = (AllocBlock) malloc(blksize);
>> +block = (AllocBlock) calloc(1, blksize);
>>
>>  /*
>>   * We could be asking for pretty big blocks here, so cope if
>> malloc

>
> I think this goes too far. You're zeroing all palloc'd memory, even if
> it's going to be passed to palloc0(), and zeroed there. It might even
> silence legitimate warnings, if there's code somewhere that does
> palloc(), and accesses some of it before initializing. Plus it's a
> performance hit.

I just did a test where I
1. memset() that block to 0xAC (aset.c:853)
2. compile and run bin/initdb, then bin/postgres
2. run an SQL file, shut down bin/postgres
3. make a copy of the transaction log file
4. change the memset() to 0x0C, repeat steps 2-3
5. compare the two transaction log files with a combination of 
hexdump(1), cut(1), and diff(1).

At the end of the output I can see:

-0f34 0010  f5ff ac02 000a  
+0f34 0010  f5ff 0c02 000a  

So it looks like the MSan complaint might be a true positive.

The SQL file is just this snippet from bit.sql:
CREATE TABLE varbit_table (a BIT VARYING(16), b BIT VARYING(16));
COPY varbit_table FROM stdin;
X0F X10
X1F X11
X2F X12
X3F X13
X8F X04
X000F   X0010
X0123   X
X2468   X2468
XFA50   X05AF
X1234   XFFF5
\.

I realize given information might be a little bit scarce, but I didn't 
know what else might be interesting to you that you wouldn't be able to 
reproduce.



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

Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

[Piotr Stefaniak]

On 2016-09-28 00:02, Andres Freund wrote:
> On 2015-09-07 17:05:10 +0100, Greg Stark wrote:
>> I feel like I remember hearing about this before but I can't find any
>> mention of it in my mail archives. It seems pretty simple to add
>> support for LLVM's Address Sanitizer (asan) by using the hooks we
>> already have for valgrind.
>
> Any plans to pick this up again?

Not remembering the context, I was initially confused about what exactly 
supposedly needs to be done in order to have ASan support, especially 
since I've been using it for a couple of years without any kind of 
modifications. Having read the whole thread now, I assume the discussion 
is now about getting MSan support, since apparently it's been already 
concluded that not much is needed for getting ASan support:

>> I don't even see any need offhand for a configure flag or autoconf
>> test. We could have a configure flag just to be consistent with
>> valgrind but it seems pointless. If you're compiling with asan I don't
>> see any reason to not use it. I'm building this to see if it works
>> now.
>
> I agree.  A flag guards Valgrind client requests, because we'd otherwise have
> no idea whether the user plans to run the binary under Valgrind.  For ASAN,
> all relevant decisions happen at build time.

Please correct me if I'm wrong.



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

[HACKERS] recovery_target_time = 'now' is not an error but still impractical setting

[Piotr Stefaniak]

First I'll describe my setup just to give you some context. If anyone
would like to discuss my ideas or propose their own ideas for
discussion, let's do so on -ADMIN or -GENERAL.

I have multiple production database clusters which I want to make
backups of. Restoring from plain dumps takes too long, so I made an
almost typical continuous archiving setup. The unusual assumption in
this case is that the standbys are all on a single machine and they are
not always running. There are multiple $PGDATA directories on the
backups machine, but only one postmaster running in standby mode,
replaying archived WAL files from each master. When it's finished
replaying them for one $PGDATA, it'll move to another. That way they all
will be sufficiently up to date while not requiring resources needed for
N replicas running all the time on a single machine. This of course
requires that the standbys are never promoted, never change the
timeline, etc. - they need to be able to keep replaying WAL files from
the masters.

I've achieved what I wanted essentially by setting standby_mode = on and
restore_command = 'cp /archivedir/%f "%p" || { pg_ctl -D . stop && false
; }', but I was looking for a more elegant solution. Which brings us to
the topic.

One thing I tried was a combination of recovery_target_action =
'shutdown' and recovery_target_time = 'now'. The result is surprising,
because then the standby tries to set the target to year 2000. That's
because recovery_target_time depends on timestamptz_in(), the result of
which can depend on GetCurrentTransactionStartTimestamp(). But at that
point there isn't any transaction yet. Which is why I'm getting
"starting point-in-time recovery to 2000-01-01 01:00:00+01".

At the very least, I think timestamptz_in() should either complain about
being called outside of transaction or return the expected value,
because returning year 2000 is unuseful at best. I would also like to
become able to do what I'm doing in a less hacky way (assuming there
isn't one already but I may be wrong), perhaps once there is a new
'furthest' setting for recovery_target or when recovery_target_time =
'now' works as I expected.

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

[HACKERS] The error message "sorry, too many clients already" is imprecise

[Piotr Stefaniak]

I recently started getting the "sorry, too many clients already" error.
There are currently four places that can generate it, but fortunately
log_error_verbosity was set to verbose so I was able to see that in this
case the warning was generated by proc.c:InitProcess().

But it's still not much, because there are three different lists you can
run out of and get the same error message from InitProcess(). I was
lucky to be able to rule out two of them. max_connections is set to much
more than the sum of possible connections from all relevant pgBouncer
instances we have, so it's not hitting max_connections. Also, this is on
9.4 and we don't use any funny extensions, so it's probably not running
out of bgworkerFreeProcs either. autovacFreeProcs is what's left but
this is still just a guess and I'd prefer to actually know.

I've made a hack for myself (attached diff against 9.4) which adds a
DETAIL-level message telling me which proc list was saturated. It's not
committable in its current form because of a C99 feature and perhaps for
other reasons.

By the way, I've also noticed that the InitProcess() can segfault upon
hitting set_spins_per_delay(procglobal->spins_per_delay). This only
happens when I run REL9_4_STABLE under gdb, set a breakpoint on
InitProcess, see an autovacuum launcher hit the breakpoint and tell gdb
to continue. "p procglobal->spins_per_delay" says "Cannot access memory
at address 0xf01b2f90". Maybe this means nothing.
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index e608198..613b36a 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -279,6 +279,14 @@ InitProcess(void)
 {
 	/* use volatile pointer to prevent code rearrangement */
 	volatile PROC_HDR *procglobal = ProcGlobal;
+#define LIST_TYPE_ENTRY(e) [(e)] = (#e)
+	enum listType { autovac, bgworker, backend } which;
+	char const * const listTypeNames[] = {
+		LIST_TYPE_ENTRY(autovac),
+		LIST_TYPE_ENTRY(bgworker),
+		LIST_TYPE_ENTRY(backend)
+	};
+#undef LIST_TYPE_ENTRY
 
 	/*
 	 * ProcGlobal should be set up already (if we are a backend, we inherit
@@ -309,11 +317,11 @@ InitProcess(void)
 	set_spins_per_delay(procglobal->spins_per_delay);
 
 	if (IsAnyAutoVacuumProcess())
-		MyProc = procglobal->autovacFreeProcs;
+		which = autovac, MyProc = procglobal->autovacFreeProcs;
 	else if (IsBackgroundWorker)
-		MyProc = procglobal->bgworkerFreeProcs;
+		which = bgworker, MyProc = procglobal->bgworkerFreeProcs;
 	else
-		MyProc = procglobal->freeProcs;
+		which = backend, MyProc = procglobal->freeProcs;
 
 	if (MyProc != NULL)
 	{
@@ -330,13 +338,13 @@ InitProcess(void)
 		/*
 		 * If we reach here, all the PGPROCs are in use.  This is one of the
 		 * possible places to detect "too many backends", so give the standard
-		 * error message.  XXX do we need to give a different failure message
-		 * in the autovacuum case?
+		 * error message.
 		 */
 		SpinLockRelease(ProcStructLock);
 		ereport(FATAL,
 (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("sorry, too many clients already")));
+ errmsg("sorry, too many clients already"),
+ errdetail("%s proc list saturated", listTypeNames[which])));
 	}
 	MyPgXact = >allPgXact[MyProc->pgprocno];
 

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

Re: [HACKERS] recovery_target_time = 'now' is not an error but still impractical setting

[Piotr Stefaniak]

On 2017-08-18 20:51, Robert Haas wrote:
> On Fri, Aug 18, 2017 at 4:39 AM, Piotr Stefaniak
> <postg...@piotr-stefaniak.me> wrote:
>> On 2017-08-17 11:24, Simon Riggs wrote:
>>> Your suggestion of "furthest" is already the default behaviour.
>>>
>>> Why are you using 'now'? Why would you want to pick a randomly
>>> selected end time?
>>
>> The idea in both cases was to stop the recovery in order to prevent the
>> standby from selecting new timeline. I want to be able to continue the
>> recovery from future WAL files.  "Furthest" really meant "as far as
>> possible without completing recovery".
> 
> Can you use recovery_target_action='shutdown' instead?

The thing I've tried was a combination of recovery_target_action =
'shutdown' and recovery_target_time = 'now'. Do you mean
recovery_target_action = 'shutdown' and everything else unset (default)?
That leads to the standby selecting new timeline anyway. Adding
standby_mode = on prevents that, but then there is no shutdown. Am I
missing something?

The only way I've managed to get recovery_target_action = 'shutdown' to
work as I needed was to follow advice by Matthijs and Christoph to
combine recovery_target_action with either setting recovery_target_time
to "now" spelled in the usual, non-special way or setting
recovery_target_xid to the latest transaction ID pg_xlogdump could find.
You could also try recovery_target_lsn, but I don't have that in 9.4. I
don't like that line of thought though, so for now I'll use the
restore_command hack I mentioned in the first message of this thread.

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

Re: [HACKERS] recovery_target_time = 'now' is not an error but still impractical setting

[Piotr Stefaniak]

On 2017-08-17 11:24, Simon Riggs wrote:
> Your suggestion of "furthest" is already the default behaviour.
> 
> Why are you using 'now'? Why would you want to pick a randomly
> selected end time?

The idea in both cases was to stop the recovery in order to prevent the
standby from selecting new timeline. I want to be able to continue the
recovery from future WAL files.  "Furthest" really meant "as far as
possible without completing recovery".

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-05-17 20:31, Tom Lane wrote:
> Piotr Stefaniak <postg...@piotr-stefaniak.me> writes:
>> On 2017-05-17 19:16, Alvaro Herrera wrote:
>>> We have someone who has studied the BSD indent code and even sent us
>>> patches to fix quite a few bugs, but we've largely ignored his efforts
>>> so far.  I propose we take that indent as part of our repo, and patch it
>>> to our preferences.
> 
>> That, I assume, would be me. Coincidentally, I'm about to push my fixes
>> upstream (FreeBSD). Before that happens, my changes can be obtained from
>> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.
> 
> Yes, I was just reviewing those threads.  IIUC, the current proposal is
> to adopt FreeBSD's version of indent as being less buggy and better
> maintained than NetBSD's, and then patch it as necessary.

One of my goals here is to fix bugs in FreeBSD indent so it's easier to
develop and maintain. I've also tried hard to not introduce serious
regressions for FreeBSD which also uses indent (for some sub-projects
even automatically). This is an ongoing effort.

What I describe below I believe to have been largely achieved.

Another goal is to incorporate all custom changes that make current
pg_bsd_indent yet another, unique indent fork, into FreeBSD indent - so
that maintaining patches for some other indent by the Postgres project
wouldn't be necessary. I understand the need to have control over a copy
of it within the Postgres project but it would be nice to not
effectively fork it by carrying custom patches around.

The third significant issue I kept in my mind was to shift some
work-arounds from pgindent to indent. When I use my indent as the base
for pgindent and set its options like this:
-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb
-cli1 -sac -ts4 -cp10
I can remove most of the work-arounds written in the perl script and
still get pretty decent results. But I expect some debate over a few things.



-- 
Sent 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: Preventive maintenance in advance of pgindent run.

[Piotr Stefaniak]

On 2017-05-17 19:16, Alvaro Herrera wrote:
> Tom Lane wrote:
> 
>> Changing the pgindent rule for such cases sounds kind of promising,
>> but will anyone pursue it?
> 
> We have someone who has studied the BSD indent code and even sent us
> patches to fix quite a few bugs, but we've largely ignored his efforts
> so far.  I propose we take that indent as part of our repo, and patch it
> to our preferences.

That, I assume, would be me. Coincidentally, I'm about to push my fixes
upstream (FreeBSD). Before that happens, my changes can be obtained from
https://github.com/pstef/freebsd_indent and tested, if anyone wishes.



-- 
Sent 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: Preventive maintenance in advance of pgindent run.

[Piotr Stefaniak]

On 2017-05-17 17:55, Peter Eisentraut wrote:
> On 5/17/17 10:14, Tom Lane wrote:
>> What I was concerned about was that pgindent will reindent the second
>> line so that it's impossible to tell whether the spacing is correct.
> 
> pgindent moving string continuations to the left is a completely
> terrible behavior anyway and we should look into changing that.  Just
> look at the mess it makes out of SELECT queries in pg_dump.c.

If I remember correctly, it tries to right-align string literals to
whatever -l ("Maximum length of an output line") was set to.


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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-05-17 22:11, Tom Lane wrote:
> Piotr Stefaniak <postg...@piotr-stefaniak.me> writes:
>> The third significant issue I kept in my mind was to shift some
>> work-arounds from pgindent to indent.
> 
> Yeah, I was wondering about that too.
> 
>> When I use my indent as the base
>> for pgindent and set its options like this:
>> -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb
>> -cli1 -sac -ts4 -cp10
> 
> Ah, thanks, ignore the message I just sent asking for that ...
> 
>> I can remove most of the work-arounds written in the perl script and
>> still get pretty decent results. But I expect some debate over a few things.
> 
> ... but what parts of the perl script do you remove?  I'm trying
> to duplicate whatever results you're currently getting.

Full copy of my pgindent attached.  Changes commented below.


@@ -17,7 +17,7 @@ my $devnull= File::Spec->devnull;

 # Common indent settings
 my $indent_opts =
-  "-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro
-bbb";
+  "-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro
-bbb -cli1 -sac -ts4 -cp10";

 # indent-dependent settings
 my $extra_opts = "";

The additional options are necessary. Mostly they replace the work-arounds.


@@ -198,60 +198,16 @@ sub pre_indent
 {
my $source = shift;

-   # remove trailing whitespace
-   $source =~ s/[ \t]+$//gm;
-

I'm not sure there won't be any trailing white-space, but I've committed
changes that should limit it.


## Comments

# Convert // comments to /* */
$source =~ s!^([ \t]*)//(.*)$!$1/* $2 */!gm;

-   # 'else' followed by a single-line comment, followed by
-   # a brace on the next line confuses BSD indent, so we push
-   # the comment down to the next line, then later pull it
-   # back up again.  Add space before _PGMV or indent will add
-   # it for us.
-   # AMD: A symptom of not getting this right is that you see
errors like:
-   # FILE: ../../../src/backend/rewrite/rewriteHandler.c
-   # Error@2259:
-   # Stuff missing from end of file
-   $source =~
- s!(\}|[ \t])else[ \t]*(/\*)(.*\*/)[ \t]*$!$1else\n$2
_PGMV$3!gm;
-
-   # Indent multi-line after-'else' comment so BSD indent will move it
-   # properly. We already moved down single-line comments above.
-   # Check for '*' to make sure we are not in a single-line comment
that
-   # has other text on the line.
-   $source =~ s!(\}|[ \t])else[ \t]*(/\*[^*]*)[ \t]*$!$1else\n
$2!gm;

These are definitely fixed.


## Other

-   # Work around bug where function that defines no local variables
-   # misindents switch() case lines and line after #else.  Do not do
-   # for struct/enum.
-   my @srclines = split(/\n/, $source);
-   foreach my $lno (1 .. $#srclines)
-   {
-   my $l2 = $srclines[$lno];
-
-   # Line is only a single open brace in column 0
-   next unless $l2 =~ /^\{[ \t]*$/;
-
-   # previous line has a closing paren
-   next unless $srclines[ $lno - 1 ] =~ /\)/;
-
-   # previous line was struct, etc.
-   next
- if $srclines[ $lno - 1 ] =~
- m!=|^(struct|enum|[ \t]*typedef|extern[ \t]+"C")!;
-
-   $srclines[$lno] = "$l2\nint pgindent_func_no_var_fix;";
-   }
-   $source = join("\n", @srclines) . "\n";# make sure there's a
final \n
-

I don't have time now to double-check, but the above shouldn't be needed
anymore.


-   # Pull up single-line comment after 'else' that was pulled down
above
-   $source =~ s!else\n[ \t]+/\* _PGMV!else\t/*!g;
-
-   # Indent single-line after-'else' comment by only one tab.
-   $source =~ s!(\}|[ \t])else[ \t]+(/\*.*\*/)[ \t]*$!$1else\t$2!gm;
-
-   # Add tab before comments with no whitespace before them (on a
tab stop)
-   $source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm;
-
-   # Remove blank line between opening brace and block comment.
-   $source =~ s!(\t*\{\n)\n([ \t]+/\*)$!$1$2!gm;
-

These are almost definitely fixed in indent.


-   # cpp conditionals
-
-   # Reduce whitespace between #endif and comments to one tab
-   $source =~ s!^\#endif[ \t]+/\*!#endif   /*!gm;
-
## Functions

-   # Work around misindenting of function with no variables defined.
-   $source =~ s!^[ \t]*int[ \t]+pgindent_func_no_var_fix;[
\t]*\n{1,2}!!gm;
-

These have likely been fixed.


-   ## Other
-
-   # Remove too much indenting after closing brace.
-   $source =~ s!^\}\t[ \t]+!}\t!gm;
-
-   # Workaround indent bug that places excessive space before 'static'.
-   $source =~ s!^static[ \t]+!static !gm;
-
-   # Remove leading whitespace from typede

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-05-22 01:50, Tom Lane wrote:
> Being lazy, I just wiped my copy and re-cloned, but it still seems the
> same as before ... last commit on the pass3 branch is from Mar 4.
> What branch should I be paying attention to?

Sorry for the trouble, this is because I interactively git-rebased it in
order to rewrite history. I do this to limit the number of commits to
the FreeBSD repository. Next time I'll leave fix-ups in chronological
order and meld them with what they fix just before committing to the
FreeBSD repository.

pass3 is the right branch. A fresh clone should have worked as in the
attached log.
me@t520 /tmp> git clone https://github.com/pstef/freebsd_indent
Cloning into 'freebsd_indent'...
remote: Counting objects: 640, done.
remote: Compressing objects: 100% (55/55), done.
remote: Total 640 (delta 128), reused 151 (delta 116), pack-reused 469
Receiving objects: 100% (640/640), 270.61 KiB | 66.00 KiB/s, done.
Resolving deltas: 100% (409/409), done.
Checking connectivity... done.
me@t520 /tmp> cd freebsd_indent
me@t520 /t/freebsd_indent> bmake CC=clang CFLAGS='-D__FBSDID="static char 
*rcsid=" -g -O0'
clang -D__FBSDID="static char *rcsid=" -g -O0-c indent.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c io.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c lexi.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c parse.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c pr_comment.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c args.c
clang   -o indent  indent.o io.o lexi.o parse.o pr_comment.o args.o 
nroff -man indent.1 > indent.cat1
me@t520 /t/freebsd_indent> cp ~/postgres/freebsd_indent/test.c .
me@t520 /t/freebsd_indent> ./indent -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 
-i4 -l79 -lp -nip -npro -bbb -cli1 
-U/home/me/pgindent-test/git/src/tools/pgindent/typedefs.list < test.c
typedef struct GinBtreeData
{
/* search methods */
BlockNumber (*findChildPage) (GinBtree, GinBtreeStack *);
BlockNumber (*getLeftMostChild) (GinBtree, Page);
bool(*isMoveRight) (GinBtree, Page);
bool(*findItem) (GinBtree, GinBtreeStack *);

/* insert methods */
OffsetNumber (*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
GinPlaceToPageRC (*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, 
void *, BlockNumber, void **, Page *, Page *);
void(*execPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, 
BlockNumber, void *);
}


void
hardclock_device_poll(void)
{
const unsigned (*TABLE_index)[2];

if (stat(pg_data, ) != 0)
{
/*
 * The Linux Standard Base Core Specification 3.1 says this should
 * return '4, program or service status is unknown'
 * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g
 * eneric/iniscrptact.html
 */
exit(is_status_request ? 4 : 1);
}
}
me@t520 /t/freebsd_indent> cat test.c
typedef struct GinBtreeData
{
/* search methods */
BlockNumber (*findChildPage) (GinBtree, GinBtreeStack *);
BlockNumber (*getLeftMostChild) (GinBtree, Page);
bool(*isMoveRight) (GinBtree, Page);
bool(*findItem) (GinBtree, GinBtreeStack *);

/* insert methods */
OffsetNumber (*findChildPtr) (GinBtree, Page, BlockNumber, 
OffsetNumber);
GinPlaceToPageRC (*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack 
*, void *, BlockNumber, void **, Page *, Page *);
void(*execPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, 
void *, BlockNumber, void *);
}


void
hardclock_device_poll(void)
{
const unsigned (*TABLE_index)[2];
if (stat(pg_data, ) != 0)
{
/*
 * The Linux Standard Base Core Specification 3.1 says this 
should
 * return '4, program or service status is unknown'
 * 
https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g
 * eneric/iniscrptact.html
 */
exit(is_status_request ? 4 : 1);
}
}
me@t520 /t/freebsd_indent>
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-05-21 03:00, Tom Lane wrote:
> I wrote:
>> Also, I found two places where an overlength comment line is simply busted
>> altogether --- notice that a character is missing at the split point:
> 
> I found the cause of that: you need to apply this patch:
> 
> --- freebsd_indent/pr_comment.c~  2017-05-17 14:59:31.548442801 -0400
> +++ freebsd_indent/pr_comment.c   2017-05-20 20:51:16.447332977 -0400
> @@ -344,8 +353,8 @@ pr_comment(void)
>   {
>   int len = strlen(t_ptr);
>  
> - CHECK_SIZE_COM(len);
> - memmove(e_com, t_ptr, len);
> + CHECK_SIZE_COM(len + 1);
> + memmove(e_com, t_ptr, len + 1);
>   last_bl = strpbrk(e_com, " \t");
>   e_com += len;
>   }
> 
> As the code stands, the strpbrk call is being applied to a
> not-null-terminated string and therefore is sometimes producing an
> insane value of last_bl, messing up decisions later in the comment.
> Having the memmove include the trailing \0 resolves that.

I have been analyzing this and came to different conclusions. Foremost,
a strpbrk() call like that finds the first occurrence of either space or
a tab, but last_bl means "last blank" - it's used for marking where to
wrap a comment line if it turns out to be too long. The previous coding
moved the character sequence byte after byte, updating last_bl every
time it was copying one of the two characters. I've rewritten that part as:
CHECK_SIZE_COM(len);
memmove(e_com, t_ptr, len);
-   last_bl = strpbrk(e_com, " \t");
e_com += len;
+   last_bl = NULL;
+   for (t_ptr = e_com - 1; t_ptr > e_com - len; t_ptr--)
+   if (*t_ptr == ' ' || *t_ptr == '\t') {
+   last_bl = t_ptr;
+   break;
+   }
}


But then I also started to wonder if there is any case when there's more
than one character to copy and I haven't found one yet. It looks like
} while (!memchr("*\n\r\b\t", *buf_ptr, 6) &&
(now_col <= adj_max_col || !last_bl));
guarantees that if we're past adj_max_col, it'll only be one non-space
character. But I'm not sure yet.



-- 
Sent 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: Preventive maintenance in advance of pgindent run.

[Piotr Stefaniak]

On 2017-05-18 18:13, Tom Lane wrote:
> Piotr Stefaniak <postg...@piotr-stefaniak.me> writes:
>> That, I assume, would be me. Coincidentally, I'm about to push my fixes
>> upstream (FreeBSD). Before that happens, my changes can be obtained from
>> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.
> 
> I spent a little bit of time on portability testing, because we are
> certainly going to insist that this tool be portable to more than
> just FreeBSD.  Things are not very good as it stands:
> 
> * Makefile is 100% BSD-specific.  Possibly we could just agree to
> disagree on that point, and include a PG-style makefile that is not
> like upstream's.  I attach the one I used for test purposes.

This would have to live outside of FreeBSD for obvious (or not) reasons.
Most likely as a part of pg_bsd_indent.  I use the original ("BSD")
Makefile on Linux, feeding it to bmake(1). But I don't expect bmake to
become a requirement for pg_bsd_indent.

> * __FBSDID() macros fail to compile anywhere else than FreeBSD.
> Couldn't you hide those inside #if 0, as you're already doing for
> the ancient sccsid strings?

The use of __FBSDID macro won't be going anywhere from FreeBSD indent,
I'm afraid. But I think it could be if-def'd out under systems other
than FreeBSD.

> * Please put the copyright[] string in indent.c inside #if 0 too,
> as that draws unreferenced-variable warnings on some compilers.
> 
> * There's one use of bcopy(), please replace with memmove().

These could probably be done upstream. I'd like to convert the strings
to comments.

> * References to  and  are problematic, as both
> are BSD-isms not found in POSIX.  They seem to be available anyway
> on Linux, but I bet not on Windows or SysV-tradition boxes.
> I removed the  includes and didn't see any problems,
> but removing  exposes calls to err() and errx(), which
> we'd have to find replacements for.  Maybe just change them to
> standard-conforming printf() + exit()?

I'll look into why indent includes sys/cdefs.h.  I don't expect to be
allowed to replace err() and errx() with equivalent solutions based on
ISO C and POSIX. I fear the idea of detecting err*() support prior to
compilation... Probably a much simpler solution would be to replace
err*() with equivalent solutions in the PG's fork of indent.  printf()
and exit() would probably be insufficient, you'd also need strerror(), I
think.


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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-05-17 23:46, Tom Lane wrote:
> I hacked around that by putting back a detab/entab step at the end
> using the existing subroutines in pgindent.  That about halved the
> size of the diff, but it's still too big to post.  Much of what
> I'm seeing with this version is randomly different decisions about
> how far to indent comments

pgindent doesn't set the -c indent option ("The column  in which comments
on code start."), so indent uses the default value of 33 (meaning column
32). If the code pushes the comment further than column 32, indent only
places a single tab between the two just to separate them.

This, given 4-column tabs, should result in placing the comment on
bitSize[INDEX_MAX_KEYS]; from your example onto column 44 - which the
newer version of indent does (if you tell it -ts4), unlike the older
one. I think that this is an improvement.

> It does seem to be handling formatting around sizeof() calls a lot better
> than the old code, as well as function pointer typedefs.  So those are
> huge wins.  But can we avoid the changes mentioned above?  I'd like the
> new version to only differ in ways that are clear improvements.

I don't know how to avoid the improvement. Try removing -ts4 as well as
putting back detab+entab.


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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-06-13 18:22, Tom Lane wrote:
> The Makefile is still BSD-ish of course, but I think
> we'll just agree to disagree there.

For compiling indent under Linux I use bmake(1). I have no problem with
including a Makefile for GNU Make in my repository.

As I understand it, there will be a copy of indent maintained by the
Postgres group - even less of a problem to include whatever you want, it
seems to me.

I think that Postgres will have to fork FreeBSD indent anyway, because
of nitems(), capsicum, __FBSDID(), etc. Now I only aim for the shortest
diff output.

> The only thing I could find to
> quibble about is that on old macOS versions I get
> 
> In file included from indent.c:49:
> indent_globs.h:222:1: warning: "STACKSIZE" redefined
> In file included from /usr/include/machine/param.h:30,
>  from /usr/include/sys/param.h:104,
>  from indent.c:42:
> /usr/include/ppc/param.h:53:1: warning: this is the location of the previous 
> definition
> 
> Maybe you could rename that symbol to IND_STACKSIZE or some such?

I just replaced it with integer constants and nitems().

> Also, I am wondering about the test cases under tests/.  I do not
> see anything in the Makefile or elsewhere suggesting how those are
> to be used.  It would sure be nice to have some quick smoke-test
> to check that a build on a new platform is working.

They'd started out like David Holland's tests for his tradcpp(1), with a
similar makefile (again, BSD make). But I was tenaciously asked to use
Kyua (a testing framework that is the standard regression test mechanism
for FreeBSD) instead, so now the makefile's existence and use is a great
secret and the file is not under any source control. Adaption of the
indent test suite to Kyua made the makefile more inelegant, but I'm
attaching it to this email in hope that you can do something useful with
it. I can only guess that you have the option to use Kyua instead, but I
don't know the tool at all.
INDENT_OBJDIR=  ${.OBJDIR:H}
INDENT= ${INDENT_OBJDIR}/indent

TESTS+= binary
TESTS+= comments
TESTS+= cppelsecom
TESTS+= declarations
TESTS+= elsecomment
TESTS+= f_decls
TESTS+= float
TESTS+= label
TESTS+= list_head
TESTS+= nsac
TESTS+= offsetof
TESTS+= parens
TESTS+= sac
TESTS+= struct
TESTS+= surplusbad
TESTS+= types_from_file
TESTS+= wchar

all: run-tests .WAIT show-diffs

$(INDENT):
${MAKE} -C ${INDENT_OBJDIR}

.for T in $(TESTS)
run-tests: $(T).diff

$(T).diff: $(T).run $(T).0.stdout $(INDENT)
-diff -u $(T).0.stdout $(T).run > $(T).diff

$(T).run: $(INDENT) $(T).0
$(INDENT) $(T).0 $(T).run -P$(T).pro 2>&1 || echo FAILED >> $(T).run
.endfor

show-diffs:
@echo '*** Test diffs ***'
.for T in $(TESTS)
@cat $(T).diff
.endfor

clean:
.for T in $(TESTS)
rm -f $(T).run $(T).diff $(T).o $(T).plist
.endfor

good:
.for T in $(TESTS)
cp $(T).run $(T).0.stdout
.endfor

.PHONY: all run-tests show-diffs clean good

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-06-13 22:23, Tom Lane wrote:
> I could not find any places where reverting this change made the
> results worse, so I'm unclear on why you made it.

I must admit I'm a bit confused about why it's not fixed yet, but I'll
have to analyze that later this week. But the idea was to convince
indent that the following is not a declaration and therefore it
shouldn't be formatted as such:

typedef void (*voidptr) (int *);

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

Re: [HACKERS] Preliminary results for proposed new pgindent implementation

[Piotr Stefaniak]

On 2017-06-16 21:56, Tom Lane wrote:
> One other thing I'd like to do while we're changing this stuff is
> to get rid of the need for entab/detab.  Right now, after doing
> all the other work, my copy of pgindent is running the code through
> detab and then entab so as to match the old decisions about how to
> represent whitespace (ie, as spaces or tabs).  This is grotty as
> can be.  I managed to tweak bsdindent so that its output matches
> what entab would do, by dint of the attached patch, which implements
> the rule "use a space instead of a tab if the tab would only move
> one column and we don't need another tab after it".  (I think entab
> is being weird with the second half of that rule, but if I remove it,
> I get circa a thousand lines of invisible whitespace changes; probably
> better not to deal with those.  With no patch at all, just letting
> bsdindent do what it does now, there's circa ten thousand changed lines.)
> 
> Unless Piotr objects, I propose to add another switch to bsdindent
> that selects this behavior, and then we can drop entab, removing
> another impediment to getting pgindent working.

I understand the reasoning, but this is a very specific need and I think
not at all universal for anyone else in the future. One of the bugs
listed in indent's manpage is that it "has more switches than ls(1)". So
currently I'm against pushing an option for the above upstream, to the
FreeBSD repository.

Why not add this to the already non-empty list of custom patches?


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

Re: [HACKERS] Preliminary results for proposed new pgindent implementation

[Piotr Stefaniak]

On 2017-06-16 20:11, Tom Lane wrote:
> Andres Freund  writes:
>> On 2017-06-16 13:44:30 -0400, Bruce Momjian wrote:
>>> Yes, it is all about <80 column output.  The current pgindent does
>>> everything possible to accomplish that --- the question is whether we
>>> want uglier code to do it.
> 
>> For me personally the misindentation is way uglier than a too long line.

> 
> I assume though that Piotr wants an option to preserve that behavior.
> I'm happy to write up a patch for bsdindent that adds a switch
> controlling this, but is there any rhyme or reason to the way its
> switches are named?

I don't want to preserve the current behavior at all, but I might need
to add an option for choosing one or the other if users of FreeBSD
indent protest.

I don't have a good name for it. The best I can do is -lpl ("-lp long
lines too").  Can I see the patch?

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

Re: [HACKERS] Preliminary results for proposed new pgindent implementation

[Piotr Stefaniak]

On 2017-06-17 00:02, Tom Lane wrote:
> Piotr Stefaniak <postg...@piotr-stefaniak.me> writes:
>> On 2017-06-16 21:56, Tom Lane wrote:
>>> Unless Piotr objects, I propose to add another switch to bsdindent
>>> that selects this behavior, and then we can drop entab, removing
>>> another impediment to getting pgindent working.
> 
>> I understand the reasoning, but this is a very specific need and I think
>> not at all universal for anyone else in the future. One of the bugs
>> listed in indent's manpage is that it "has more switches than ls(1)". So
>> currently I'm against pushing an option for the above upstream, to the
>> FreeBSD repository.
> 
>> Why not add this to the already non-empty list of custom patches?
> 
> Umm ... I thought the idea was to get to the point where the list of
> custom patches *is* empty.  Except for carrying our own Makefile of
> course.  I'd be sad if we needed a fork just for this.
> 
> What I'm testing with right now has just four differences from your repo:

There are also the "portability fixes" and they're the main problem.

I've simply removed things like capsicum or __FBSDID() because I thought
it wouldn't be a problem since Postgres will have its own copy of indent
anyway (so that its behavior is not a moving target). I can ifdef-out
them instead of removing entirely, I just didn't think it was important
anymore.

I expect to be in trouble for replacing err() and errx(), though.


> 1. This workaround for what I believe you agree is a bug:
> 
> - ps.in_decl = ps.decl_on_line = ps.last_token != type_def;
> + ps.in_decl = ps.decl_on_line = true;

That will need a proper fix...

> 2. The long-lines adjustment I just sent you a patch for.

That looks very good.

> 3. The tab-vs-space difference under discussion here.

I can be convinced to make it another option upstream. But I dislike it
nevertheless.

> 4. A temporary hack affecting the indentation of comments on the same line
> (forcing them to a multiple of 8 spaces even though tabsize is 4).  I have
> every intention of dropping that one later; I just don't want to deal with
> comment reindentation at the same time as these other things.

Great!

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

Re: [HACKERS] Preliminary results for proposed new pgindent implementation

[Piotr Stefaniak]

On 2017-06-17 21:55, Tom Lane wrote:
> I spent some time looking into this.  I reverted your commits
> 198457848ae5c86bec3336a9437dd5aa30f480c2 (Replace err.h functions with
> standard C equivalents) and fb10acb040b90bdcbad09defd303363db29257d1
> (Remove inclusion of sys/cdefs.h) locally and tried to build without
> those.

I wanted to mirror that move, but forgot to not rebase the repository,
so I removed those two commits instead of committing their negatives.
Sorry about that.

> I've successfully worked around the err.h change by adding
> cut-down versions of FreeBSD 11's err.h and err.c to the fileset
> (see attached).

I thought about something like:
#ifdef __FreeBSD__
#include 
#define ERR(...) err(__VA_ARGS__)
#define ERRX(...) errx(__VA_ARGS__)
#else
#include "err.h"
#endif

and then call ERR() and ERRX() instead of err() and errx(). But that
requires C99. And I would have a very hard time convincing anyone that
it makes any sense from FreeBSD's perspective, since indent is part of
the base system, where  is guaranteed to exist.

Perhaps it would be best for everyone if indent was moved out of FreeBSD
base, so that portability arguments would make more sense. But that
would take time and some debate.

> However, it's proving impossible to work around having
> "#include " as the first live code in the files.  I thought
> maybe we could provide a dummy cdefs.h file, but that breaks things on
> platforms where cdefs.h is a real thing and is relied on by other system
> headers --- which includes both Linux and BSD.  It seems we would have
> to have something like #ifdef HAVE_SYS_CDEFS_H, but that is already a
> departure from FreeBSD practice.

I was thinking if I could get away with putting those into #ifdef
__FreeBSD__ ... #endif. I think that it might be feasible unlike the
idea above. I could be wrong.

> So what I'm currently thinking is that we have to diverge from the
> FreeBSD sources to the extent of removing #include 
> and the __FBSDID() calls, and instead inserting #include "c.h" to
> pick up PG's own portability definitions.  The thing that forced me
> into the latter is that there seems no way to avoid compiler warnings
> if we don't decorate the declarations of err() and errx() with noreturn
> and printf-format attributes --- and we need c.h to provide portable
> ways of writing those.  But there are probably other portability things
> that we'll need c.h for, anyway, especially if we want to make it work
> on Windows.  So I'm thinking this is a small and easily maintainable
> difference from the upstream FreeBSD files.

That works for me.

> When I inserted #include "c.h", I got duplicate-macro-definition warnings
> about "true" and "false", so I would ask you to add this:

Done.



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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

>>> * the comments get formatted differently for -ts4 than -ts8

Still haven't put any thought into it, so I still don't know what to do
here.

>>> * extra spacing getting inserted for fairly long labels

I think the fix is as easy as not producing the space. I committed that.

>>> * some enum declarations get the phony extra indentation
>>> * surplus indentation for SH_ELEMENT_TYPE * data;

I think this is now fixed.

>>> * comments on the same line are now run together
Indent has worked like for a long time; current pg_bsd_indent does that
as well. It was now uncovered by my removing this line from pgindent:

# Add tab before comments with no whitespace before them (on a tab stop)
$source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm;

> There's also the portability issues: __FBSDID() and bcopy() and
>  [and err.h].

I think that's fixed as well.


I've never been too excited to improve indent and it's increasingly
challenging for me to force myself to work on it now, after I've
invested so much of my spare time into it. So please bear with me if
there are any errors.

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-06-14 19:31, Tom Lane wrote:
> Does that test case pass for you?

No, I broke it recently. Sorry.

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

On 2017-06-14 17:05, Bruce Momjian wrote:
> On Wed, Jun 14, 2017 at 10:38:40AM -0400, Tom Lane wrote:
>> btw, I was slightly amused to notice that this version still calls itself
>>
>> $ indent -V
>> pg_bsd_indent 1.3
>>
>> Don't know what you plan to do with that program name, but we certainly
>> need a version number bump so that pgindent can tell that it's got an
>> up-to-date copy.  1.4?  2.0?
> 
> For Piotr's reference, we will update src/tools/pgindent/pgindent to
> match whatever new version number you use.

I would like to go a bit further than that. I see that GNU indent
doesn't recognize -V, but prints its version if you use the option
--version. I wish to implement the same option for FreeBSD indent, but
that would force a change in how pgindent recognizes indent.

As for the version bump, I briefly considered "9.7.0", but 2.0 seems
more appropriate as the 2 would reflect the fundamental changes that
I've done and the .0 would indicate that it's still rough around edges.

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

[Piotr Stefaniak]

> * const unsigned(*TABLE_index)[2];
> * OffsetNumber(*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
> * an overlength comment line is simply busted altogether

Fixed and pushed to my github repository. Note that indent won't wrap
long lines like links or paths anymore. But obviously it can't join
parts of long links that have been wrapped by previous pgindent runs.

> * the comments get formatted differently for -ts4 than -ts8
> * extra spacing getting inserted for fairly long labels
> * some enum declarations get the phony extra indentation
> * comments on the same line are now run together
> * surplus indentation for SH_ELEMENT_TYPE * data;

Please tell me if the list of your complaints above is incomplete. I
will analyze those next.


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

Re: [HACKERS] SQL/JSON in PostgreSQL

[Piotr Stefaniak]

On 2017-02-28 20:08, Oleg Bartunov wrote:
> Attached patch is an implementation of SQL/JSON data model from SQL-2016
> standard (ISO/IEC 9075-2:2016(E))

I've faintly started looking into this.

> We created repository for reviewing (ask for write access) -
> https://github.com/postgrespro/sqljson/tree/sqljson

> Examples of usage can be found in src/test/regress/sql/sql_json.sql

> The whole documentation about json support should be reorganized and added,
> and we plan to do this before release. We need help of community here.


> The standard describes SQL/JSON path language, which used by SQL/JSON query
> operators to query JSON. It defines path language as string literal. We
> implemented the path language as  JSONPATH data type, since other
> approaches are not friendly to planner and executor.

I was a bit sad to discover that I can't
PREPARE jsq AS SELECT JSON_QUERY('{}', $1);
I assume because of this part of the updated grammar:
json_path_specification:
Sconst { $$ = $1; }
   ;

Would it make sense, fundamentally, to allow variables there? After 
Andrew Gierth's analysis of this grammar problem, I understand that it's 
not reasonable to expect JSON_TABLE() to support variable jsonpaths, but 
maybe it would be feasible for everything else? From Andrew's changes to 
the new grammar (see attached) it seems to me that at least that part is 
possible. Or should I forget about trying to implement the other part?
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index adfe9b1..f459996 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -624,6 +624,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	json_table_plan_cross
 	json_table_plan_primary
 	json_table_default_plan
+	json_path_specification
 
 %type 		json_arguments
 	json_passing_clause_opt
@@ -635,8 +636,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type 		json_returning_clause_opt
 
-%type 			json_path_specification
-	json_table_column_path_specification_clause_opt
+%type 			json_table_column_path_specification_clause_opt
 	json_table_path_name
 	json_as_path_name_clause_opt
 
@@ -845,6 +845,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
  */
 %nonassoc	UNBOUNDED		/* ideally should have same precedence as IDENT */
 %nonassoc	ERROR_P EMPTY_P DEFAULT ABSENT /* JSON error/empty behavior */
+%nonassoc	COLUMNS FALSE_P KEEP OMIT PASSING TRUE_P UNKNOWN
 %nonassoc	IDENT GENERATED NULL_P PARTITION RANGE ROWS PRECEDING FOLLOWING CUBE ROLLUP
 %left		Op OPERATOR		/* multi-character ops and user-defined operators */
 %left		'+' '-'
@@ -14472,7 +14473,7 @@ json_context_item:
 		;
 
 json_path_specification:
-			Sconst	{ $$ = $1; }
+			a_expr	{ $$ = $1; }
 		;
 
 json_as_path_name_clause_opt:
@@ -14802,7 +14803,7 @@ json_table_formatted_column_definition:
 		;
 
 json_table_nested_columns:
-			NESTED path_opt json_path_specification
+			NESTED path_opt Sconst
 			json_as_path_name_clause_opt
 			json_table_columns_clause
 {

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