Re: currawong is not a happy animal

I've not been able to build master for a few days now on visual studios due
some problems in the new test_shm_mq contrib module.
I'm not too familiar with how the visual studio project files are generated
so I've not yet managed to come up with a fix just yet.

However I have attached a fix for the many compiler warnings that currawong
has been seeing since 9c14dd2

On 01/17/2014 11:26 AM, David Rowley wrote:
> I've not been able to build master for a few days now on visual
> studios due some problems in the new test_shm_mq contrib module.
> I'm not too familiar with how the visual studio project files are
> generated so I've not yet managed to come up with a fix just yet.
>
> However I have attached a fix for the many compiler warnings
> that currawong has been seeing since 9c14dd2

That seems perfe4ctly sane, so I have committed it. I'll look into the
other issue.

cheers

andrew

On Fri, Jan 17, 2014 at 5:51 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 01/17/2014 11:26 AM, David Rowley wrote:
>
>> I've not been able to build master for a few days now on visual studios
>> due some problems in the new test_shm_mq contrib module.
>> I'm not too familiar with how the visual studio project files are
>> generated so I've not yet managed to come up with a fix just yet.
>>
>> However I have attached a fix for the many compiler warnings that
>> currawong has been seeing since 9c14dd2
>>
>
>
> That seems perfe4ctly sane, so I have committed it. I'll look into the
> other issue.
>

Thanks.

I could've sworn I committed it that way, but I see now that it's sitting
unstaged in my working directory...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 01/17/2014 11:51 AM, Andrew Dunstan wrote:
>
> On 01/17/2014 11:26 AM, David Rowley wrote:
>> I've not been able to build master for a few days now on visual
>> studios due some problems in the new test_shm_mq contrib module.
>> I'm not too familiar with how the visual studio project files are
>> generated so I've not yet managed to come up with a fix just yet.
>>
>> However I have attached a fix for the many compiler warnings that
>> currawong has been seeing since 9c14dd2
>
>
> That seems perfe4ctly sane, so I have committed it. I'll look into the
> other issue.
>
>

It looks like what we need to fix the immediate problem is to mark
set_latch_on_sigusr1 with PGDLLIMPORT. Is that the only one of these
symbols we might need to make visible?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> It looks like what we need to fix the immediate problem is to mark
> set_latch_on_sigusr1 with PGDLLIMPORT. Is that the only one of these
> symbols we might need to make visible?

We've generally relied on the buildfarm to cue us to add PGDLLIMPORT.
Add that one and see what happens ...

regards, tom lane

On 01/17/2014 12:43 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> It looks like what we need to fix the immediate problem is to mark
>> set_latch_on_sigusr1 with PGDLLIMPORT. Is that the only one of these
>> symbols we might need to make visible?
> We've generally relied on the buildfarm to cue us to add PGDLLIMPORT.
> Add that one and see what happens ...
>
>

OK, done.

cheers

andrew

On Sat, Jan 18, 2014 at 6:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 01/17/2014 12:43 PM, Tom Lane wrote:
>
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>
>>> It looks like what we need to fix the immediate problem is to mark
>>> set_latch_on_sigusr1 with PGDLLIMPORT. Is that the only one of these
>>> symbols we might need to make visible?
>>>
>> We've generally relied on the buildfarm to cue us to add PGDLLIMPORT.
>> Add that one and see what happens ...
>>
>>
>>
>
>
> OK, done.
>
>
Great, that fixes it on my end.

Thanks for fixing those.

Regards
David Rowley

> cheers
>
> andrew
>
>

On 01/17/2014 12:55 PM, David Rowley wrote:
> On Sat, Jan 18, 2014 at 6:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net
> <mailto:andrew(at)dunslane(dot)net>> wrote:
>
>
> On 01/17/2014 12:43 PM, Tom Lane wrote:
>
> Andrew Dunstan <andrew(at)dunslane(dot)net
> <mailto:andrew(at)dunslane(dot)net>> writes:
>
> It looks like what we need to fix the immediate problem is
> to mark
> set_latch_on_sigusr1 with PGDLLIMPORT. Is that the only
> one of these
> symbols we might need to make visible?
>
> We've generally relied on the buildfarm to cue us to add
> PGDLLIMPORT.
> Add that one and see what happens ...
>
>
>
>
> OK, done.
>
>
> Great, that fixes it on my end.
>
> Thanks for fixing those.

Not quite out of the woods yet. We're getting this regression failure on
Windows/MSVC (see
<http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2014-01-17%2018%3A20%3A35>):

*** c:/prog/bf/root/HEAD/pgsql.2612/contrib/test_shm_mq/expected/test_shm_mq.out Fri Jan 17 13:20:53 2014
--- c:/prog/bf/root/HEAD/pgsql.2612/contrib/test_shm_mq/results/test_shm_mq.out Fri Jan 17 13:44:33 2014
***************
*** 5,18 ****
-- internal sanity tests fail.
--
SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 10000, 1);
! test_shm_mq
! -------------
!
! (1 row)
!
SELECT test_shm_mq_pipelined(16384, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,270000)), 200, 3);
! test_shm_mq_pipelined
! -----------------------
!
! (1 row)
!
--- 5,10 ----
-- internal sanity tests fail.
--
SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 10000, 1);
! ERROR: queue size must be at least 472262143 bytes
SELECT test_shm_mq_pipelined(16384, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,270000)), 200, 3);
! ERROR: queue size must be at least 472262143 bytes

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Not quite out of the woods yet. We're getting this regression failure on
> Windows/MSVC (see
> <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2014-01-17%2018%3A20%3A35>):

> SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 10000, 1);
> ! ERROR: queue size must be at least 472262143 bytes

It looks like this is computing a bogus value:

const Size shm_mq_minimum_size =
MAXALIGN(offsetof(shm_mq, mq_ring)) + MAXIMUM_ALIGNOF;

I seem to recall that we've previously found that you have to write

MAXALIGN(offsetof(shm_mq, mq_ring[0])) + MAXIMUM_ALIGNOF;

to keep MSVC happy with a reference to an array member in offsetof.

regards, tom lane

Tom Lane escribió:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > Not quite out of the woods yet. We're getting this regression failure on
> > Windows/MSVC (see
> > <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2014-01-17%2018%3A20%3A35>):
>
> > SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 10000, 1);
> > ! ERROR: queue size must be at least 472262143 bytes
>
> It looks like this is computing a bogus value:
>
> const Size shm_mq_minimum_size =
> MAXALIGN(offsetof(shm_mq, mq_ring)) + MAXIMUM_ALIGNOF;
>
> I seem to recall that we've previously found that you have to write
>
> MAXALIGN(offsetof(shm_mq, mq_ring[0])) + MAXIMUM_ALIGNOF;
>
> to keep MSVC happy with a reference to an array member in offsetof.

Hmm, this seems to contradict what's documented at the definition of
FLEXIBLE_ARRAY_MEMBER:

/* Define to nothing if C supports flexible array members, and to 1 if it does
not. That way, with a declaration like `struct s { int n; double
d[FLEXIBLE_ARRAY_MEMBER]; };', the struct hack can be used with pre-C99
compilers. When computing the size of such an object, don't use 'sizeof
(struct s)' as it overestimates the size. Use 'offsetof (struct s, d)'
instead. Don't use 'offsetof (struct s, d[0])', as this doesn't work with
MSVC and with C++ compilers. */
#define FLEXIBLE_ARRAY_MEMBER /**/

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

On 01/17/2014 02:54 PM, Alvaro Herrera wrote:
> Tom Lane escribió:
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> Not quite out of the woods yet. We're getting this regression failure on
>>> Windows/MSVC (see
>>> <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2014-01-17%2018%3A20%3A35>):
>>> SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 10000, 1);
>>> ! ERROR: queue size must be at least 472262143 bytes
>> It looks like this is computing a bogus value:
>>
>> const Size shm_mq_minimum_size =
>> MAXALIGN(offsetof(shm_mq, mq_ring)) + MAXIMUM_ALIGNOF;
>>
>> I seem to recall that we've previously found that you have to write
>>
>> MAXALIGN(offsetof(shm_mq, mq_ring[0])) + MAXIMUM_ALIGNOF;
>>
>> to keep MSVC happy with a reference to an array member in offsetof.
> Hmm, this seems to contradict what's documented at the definition of
> FLEXIBLE_ARRAY_MEMBER:
>
> /* Define to nothing if C supports flexible array members, and to 1 if it does
> not. That way, with a declaration like `struct s { int n; double
> d[FLEXIBLE_ARRAY_MEMBER]; };', the struct hack can be used with pre-C99
> compilers. When computing the size of such an object, don't use 'sizeof
> (struct s)' as it overestimates the size. Use 'offsetof (struct s, d)'
> instead. Don't use 'offsetof (struct s, d[0])', as this doesn't work with
> MSVC and with C++ compilers. */
> #define FLEXIBLE_ARRAY_MEMBER /**/
>
>

Well, there's a bunch of these in the source:

[andrew(at)emma pg_head]$ grep -r offsetof.*\\[0 src/backend
src/backend/access/nbtree/nbtutils.c: size = offsetof(BTVacInfo,
vacuums[0]);
src/backend/utils/adt/geo_ops.c: size = offsetof(PATH, p[0])
+sizeof(path->p[0]) * npts;
src/backend/utils/adt/geo_ops.c: if (npts <= 0 || npts >= (int32)
((INT_MAX - offsetof(PATH, p[0])) / sizeof(Point)))
src/backend/utils/adt/geo_ops.c: size = offsetof(PATH, p[0])
+sizeof(path->p[0]) * npts;
src/backend/utils/adt/geo_ops.c: size = offsetof(POLYGON, p[0])
+sizeof(poly->p[0]) * npts;
src/backend/utils/adt/geo_ops.c: if (npts <= 0 || npts >= (int32)
((INT_MAX - offsetof(POLYGON, p[0])) / sizeof(Point)))
src/backend/utils/adt/geo_ops.c: size = offsetof(POLYGON, p[0])
+sizeof(poly->p[0]) * npts;
src/backend/utils/adt/geo_ops.c: size = offsetof(PATH, p[0])
+base_size;
src/backend/utils/adt/geo_ops.c: size = offsetof(POLYGON, p[0])
+sizeof(poly->p[0]) * path->npts;
src/backend/utils/adt/geo_ops.c: size = offsetof(POLYGON, p[0])
+sizeof(poly->p[0]) * 4;
src/backend/utils/adt/geo_ops.c: size = offsetof(PATH, p[0])
+sizeof(path->p[0]) * poly->npts;
src/backend/utils/adt/geo_ops.c: size = offsetof(POLYGON, p[0])
+base_size;
src/backend/postmaster/pgstat.c: len =
offsetof(PgStat_MsgTabstat, m_entry[0]) +
src/backend/postmaster/pgstat.c: pgstat_send(&msg,
offsetof(PgStat_MsgFuncstat, m_entry[0]) +
src/backend/postmaster/pgstat.c: pgstat_send(&msg,
offsetof(PgStat_MsgFuncstat, m_entry[0]) +
src/backend/postmaster/pgstat.c: len =
offsetof(PgStat_MsgTabpurge, m_tableid[0])
src/backend/postmaster/pgstat.c: len =
offsetof(PgStat_MsgTabpurge, m_tableid[0])
src/backend/postmaster/pgstat.c: len =
offsetof(PgStat_MsgFuncpurge, m_functionid[0])
src/backend/postmaster/pgstat.c: len =
offsetof(PgStat_MsgFuncpurge, m_functionid[0])
src/backend/postmaster/pgstat.c: len =
offsetof(PgStat_MsgTabpurge, m_tableid[0]) +sizeof(Oid);

cheers

andrew

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Tom Lane escribi:
>> I seem to recall that we've previously found that you have to write
>> MAXALIGN(offsetof(shm_mq, mq_ring[0])) + MAXIMUM_ALIGNOF;
>> to keep MSVC happy with a reference to an array member in offsetof.

> Hmm, this seems to contradict what's documented at the definition of
> FLEXIBLE_ARRAY_MEMBER:

Ah, I thought we had that issue documented somewhere, but failed to find
this comment, or I'd have known that was backwards.

The other possibility I was contemplating is that "export a const
variable" doesn't actually work for some reason. We're not in the habit
of doing that elsewhere, so I don't find that theory outlandish. Perhaps
it could be fixed by adding PGDLLIMPORT to the extern, but on the whole
I'd rather avoid the technique altogether.

The least-unlike-other-Postgres-code approach would be to go ahead and
export the struct so that the size computation could be provided as a
#define in the same header. Robert stated a couple days ago that he
didn't foresee much churn in this struct, so that doesn't seem
unacceptable.

Another possibility is to refactor so that testing an allocation request
against shm_mq_minimum_size is the responsibility of storage/ipc/shm_mq.c,
not some random code in a contrib module. It's not immediately apparent
to me why it's good code modularization to have a contrib module
responsible for checking sizes based on the sizeof a struct it's not
supposed to have any access to.

regards, tom lane

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/17/2014 02:54 PM, Alvaro Herrera wrote:
>> Hmm, this seems to contradict what's documented at the definition of
>> FLEXIBLE_ARRAY_MEMBER:

> Well, there's a bunch of these in the source:

Yeah, I did the same grep and noted the same thing --- but on second look,
I think those are all structs that are defined without use of
FLEXIBLE_ARRAY_MEMBER, which OTOH *is* used in struct shm_mq.

regards, tom lane

Andrew Dunstan escribió:
>
> On 01/17/2014 02:54 PM, Alvaro Herrera wrote:

> >Hmm, this seems to contradict what's documented at the definition of
> >FLEXIBLE_ARRAY_MEMBER:

> Well, there's a bunch of these in the source:

AFAICT these are all defined with non-zero, non-empty array sizes, not
FLEXIBLE_ARRAY_MEMBER (which mq_ring is). I don't know why that makes a
difference but apparently it does.

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

On 01/17/2014 03:15 PM, Tom Lane wrote:

> The other possibility I was contemplating is that "export a const
> variable" doesn't actually work for some reason. We're not in the habit
> of doing that elsewhere, so I don't find that theory outlandish. Perhaps
> it could be fixed by adding PGDLLIMPORT to the extern, but on the whole
> I'd rather avoid the technique altogether.
>
> The least-unlike-other-Postgres-code approach would be to go ahead and
> export the struct so that the size computation could be provided as a
> #define in the same header. Robert stated a couple days ago that he
> didn't foresee much churn in this struct, so that doesn't seem
> unacceptable.
>
> Another possibility is to refactor so that testing an allocation request
> against shm_mq_minimum_size is the responsibility of storage/ipc/shm_mq.c,
> not some random code in a contrib module. It's not immediately apparent
> to me why it's good code modularization to have a contrib module
> responsible for checking sizes based on the sizeof a struct it's not
> supposed to have any access to.
>
>

Or maybe we could expose the value via a function rather than a const
variable.

cheers

andrew

On Sat, Jan 18, 2014 at 2:48 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/17/2014 03:15 PM, Tom Lane wrote:
>
>> The other possibility I was contemplating is that "export a const
>> variable" doesn't actually work for some reason. We're not in the habit
>> of doing that elsewhere, so I don't find that theory outlandish. Perhaps
>> it could be fixed by adding PGDLLIMPORT to the extern, but on the whole
>> I'd rather avoid the technique altogether.

Is there any specific reason why adding PGDLLIMPORT for exporting
const variables is not good, when we are already doing for other variables
like InterruptHoldoffCount, CritSectionCount?

On searching in code, I found that for few const variables we do
extern PGDLLIMPORT. For example:
extern PGDLLIMPORT const int NumScanKeywords;
extern PGDLLIMPORT const char *debug_query_string;

>> The least-unlike-other-Postgres-code approach would be to go ahead and
>> export the struct so that the size computation could be provided as a
>> #define in the same header. Robert stated a couple days ago that he
>> didn't foresee much churn in this struct, so that doesn't seem
>> unacceptable.
>>
>> Another possibility is to refactor so that testing an allocation request
>> against shm_mq_minimum_size is the responsibility of storage/ipc/shm_mq.c,
>> not some random code in a contrib module. It's not immediately apparent
>> to me why it's good code modularization to have a contrib module
>> responsible for checking sizes based on the sizeof a struct it's not
>> supposed to have any access to.
>
> Or maybe we could expose the value via a function rather than a const
> variable.

All of above suggested alternatives will fix this problem. However after
fixing this problem, when I ran the test further it crashed the bgworker
and I found that reason was there are some other variables
(ImmediateInterruptOK, MyBgworkerEntry) used in test module without
PGDLLIMPORT.

After adding PGDLLIMPORT to variables (ImmediateInterruptOK,
MyBgworkerEntry, shm_mq_minimum_size) both the tests defined in
contrib module passed.

Attached patch fixes the problems related to test_shm_mq for me.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 01/18/2014 02:42 AM, Amit Kapila wrote:
> On Sat, Jan 18, 2014 at 2:48 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 01/17/2014 03:15 PM, Tom Lane wrote:
>>
>>> The other possibility I was contemplating is that "export a const
>>> variable" doesn't actually work for some reason. We're not in the habit
>>> of doing that elsewhere, so I don't find that theory outlandish. Perhaps
>>> it could be fixed by adding PGDLLIMPORT to the extern, but on the whole
>>> I'd rather avoid the technique altogether.
> Is there any specific reason why adding PGDLLIMPORT for exporting
> const variables is not good, when we are already doing for other variables
> like InterruptHoldoffCount, CritSectionCount?
>
> On searching in code, I found that for few const variables we do
> extern PGDLLIMPORT. For example:
> extern PGDLLIMPORT const int NumScanKeywords;
> extern PGDLLIMPORT const char *debug_query_string;
>

[...]
> After adding PGDLLIMPORT to variables (ImmediateInterruptOK,
> MyBgworkerEntry, shm_mq_minimum_size) both the tests defined in
> contrib module passed.
>
>
> Attached patch fixes the problems related to test_shm_mq for me.
>

OK, I'll apply this later today unless there are strong objections. That
would get the buildfarm green again and we could argue about the rest
later if necessary.

cheers

andrew

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> Is there any specific reason why adding PGDLLIMPORT for exporting
> const variables is not good, when we are already doing for other variables
> like InterruptHoldoffCount, CritSectionCount?

> On searching in code, I found that for few const variables we do
> extern PGDLLIMPORT. For example:
> extern PGDLLIMPORT const int NumScanKeywords;

OK, that one is a direct counterexample to my worry.

I'm still unconvinced that having a contrib module checking stuff based
on the size of a struct it's not supposed to access represents a sane
division of responsibilities; but let's fix the build problem now and
then Robert can contemplate that question at his leisure.

regards, tom lane

On 01/18/2014 12:26 AM, David Rowley wrote:
> -#if defined(_WIN32)
> +#if defined(_WIN32) && !defined(WIN32)

That makes sense, since we force WIN32 in the build system. I should've
seen that. Thanks for catching it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Jan 18, 2014 at 11:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> Is there any specific reason why adding PGDLLIMPORT for exporting
>> const variables is not good, when we are already doing for other variables
>> like InterruptHoldoffCount, CritSectionCount?
>
>> On searching in code, I found that for few const variables we do
>> extern PGDLLIMPORT. For example:
>> extern PGDLLIMPORT const int NumScanKeywords;
>
> OK, that one is a direct counterexample to my worry.
>
> I'm still unconvinced that having a contrib module checking stuff based
> on the size of a struct it's not supposed to access represents a sane
> division of responsibilities; but let's fix the build problem now and
> then Robert can contemplate that question at his leisure.

Exposing the whole struct because of that minor detail seemed really
ugly to me, and I feel strongly that we should avoid it. In the
intended use of this code, I expect queues to be some number of
kilobytes on the low end up to some number of megabytes on the high
end, so the fact that it can't be less than 56 bytes or so is sort of
a meaningless blip on the radar. So one thing I thought about is just
defining the minimum size as something conservative, like 1024. But
for testing purposes, it was certainly useful to create the smallest
possible queue, because that stresses the synchronization code to the
maximum degree possible. So on the whole I'm still happy with how I
did this.

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