Re: Retain dynamic shared memory segments for postmaster lifetime

Currently there is no way user can keep the dsm
segments if he wants for postmaster lifetime, so I
have exposed a new API dsm_keep_segment()
to implement the same.

The specs and need for this API is already discussed
in thread:
http://www.postgresql.org/message-id/CA+TgmoaKoGuJQbEdGeYKYSXud9EAidqx77J2_HXzRgFo3Hr46A@mail.gmail.com

I had used dsm_demo (hacked it a bit) module used
during initial tests for dsm API's to verify the working on
Windows. So one idea could be that I can extend
that module to use this new API, so that it can be tested
by others as well or if you have any other better way, please
do let me know.

As the discussion about its specs and need is already
discussed in above mentioned thread, so I will upload
this patch to CF unless there is any Objection.

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

On Sun, Jan 12, 2014 at 12:41 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Currently there is no way user can keep the dsm
> segments if he wants for postmaster lifetime, so I
> have exposed a new API dsm_keep_segment()
> to implement the same.
>
> The specs and need for this API is already discussed
> in thread:
> http://www.postgresql.org/message-id/CA+TgmoaKoGuJQbEdGeYKYSXud9EAidqx77J2_HXzRgFo3Hr46A@mail.gmail.com

We have decided to bump reference count for segment
and call DuplicateHandle for Windows, but I think it should
also do what dsm_keep_mapping() does that is
ResourceOwnerForgetDSM(), else it will give
Warning: dynamic shared memory leak at transaction end.

> I had used dsm_demo (hacked it a bit) module used
> during initial tests for dsm API's to verify the working on
> Windows. So one idea could be that I can extend
> that module to use this new API, so that it can be tested
> by others as well or if you have any other better way, please
> do let me know.

I have extended test (contrib) module dsm_demo such that now user
can specify during dsm_demo_create the lifespan of segment.
The values it can accept are 0 or 1. Default value is 0.
0 -- means segment will be accessible for session life time
1 -- means segment will be accessible for postmaster life time

The behaviour is as below:
Test -1 (Session life time)
Session - 1
-- here it will create segment for session lifetime
select dsm_demo_create('this message is from session-1', 0);
dsm_demo_create
-----------------
827121111

Session - 2
-----------------
select dsm_demo_read(827121111);
dsm_demo_read
----------------------------
this message is from session-1
(1 row)

Session-1
\q

Session-2
postgres=# select dsm_demo_read(827121111);
dsm_demo_read
---------------

(1 row)

Conclusion of Test-1 : As soon as session which has created segment finished,
the segment becomes non-accessible.

Test -2 (Postmaster life time)
Session - 1
-- here it will create segment for postmaster lifetime
select dsm_demo_create('this message is from session-1', 1);
dsm_demo_create
-----------------
827121111

Session - 2
-----------------
select dsm_demo_read(827121111);
dsm_demo_read
----------------------------
this message is from session-1
(1 row)

Session-1
\q

Session-2
postgres=# select dsm_demo_read(827121111);
dsm_demo_read
---------------
this message is from session-1
(1 row)

Conclusion of Test-2 : a. Segment is accessible for postmaster lifetime.
b. if user restart server, segment is
not accessible.

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

On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I have extended test (contrib) module dsm_demo such that now user
> can specify during dsm_demo_create the lifespan of segment.
> The values it can accept are 0 or 1. Default value is 0.
> 0 -- means segment will be accessible for session life time
> 1 -- means segment will be accessible for postmaster life time
>
>
> The behaviour is as below:
> Test -1 (Session life time)
> Session - 1
> -- here it will create segment for session lifetime
> select dsm_demo_create('this message is from session-1', 0);
> dsm_demo_create
> -----------------
> 827121111
>
> Session - 2
> -----------------
> select dsm_demo_read(827121111);
> dsm_demo_read
> ----------------------------
> this message is from session-1
> (1 row)
>
>
> Session-1
> \q
>
> Session-2
> postgres=# select dsm_demo_read(827121111);
> dsm_demo_read
> ---------------
>
> (1 row)
>
> Conclusion of Test-1 : As soon as session which has created segment finished,
> the segment becomes non-accessible.
>
>
> Test -2 (Postmaster life time)
> Session - 1
> -- here it will create segment for postmaster lifetime
> select dsm_demo_create('this message is from session-1', 1);
> dsm_demo_create
> -----------------
> 827121111
>
> Session - 2
> -----------------
> select dsm_demo_read(827121111);
> dsm_demo_read
> ----------------------------
> this message is from session-1
> (1 row)
>
>
> Session-1
> \q
>
> Session-2
> postgres=# select dsm_demo_read(827121111);
> dsm_demo_read
> ---------------
> this message is from session-1
> (1 row)
>
> Conclusion of Test-2 : a. Segment is accessible for postmaster lifetime.
> b. if user restart server, segment is
> not accessible.
>
>

Applied dsm_keep_segment_v1.patch and dsm_demo_v1.patch.
Got the following warning when I tried above example:

postgres=# select dsm_demo_create('this message is from session-new', 1);
WARNING: dynamic shared memory leak: segment 1402373971 still referenced
WARNING: dynamic shared memory leak: segment 1402373971 still referenced
dsm_demo_create
-----------------
1402373971
(1 row)

--
Amit

On Mon, Jan 27, 2014 at 11:18 PM, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I have extended test (contrib) module dsm_demo such that now user
>> can specify during dsm_demo_create the lifespan of segment.
>> The values it can accept are 0 or 1. Default value is 0.
>> 0 -- means segment will be accessible for session life time
>> 1 -- means segment will be accessible for postmaster life time
>>
>>
>> The behaviour is as below:
>> Test -1 (Session life time)
>> Session - 1
>> -- here it will create segment for session lifetime
>> select dsm_demo_create('this message is from session-1', 0);
>> dsm_demo_create
>> -----------------
>> 827121111
>>
>> Session - 2
>> -----------------
>> select dsm_demo_read(827121111);
>> dsm_demo_read
>> ----------------------------
>> this message is from session-1
>> (1 row)
>>
>>
>> Session-1
>> \q
>>
>> Session-2
>> postgres=# select dsm_demo_read(827121111);
>> dsm_demo_read
>> ---------------
>>
>> (1 row)
>>
>> Conclusion of Test-1 : As soon as session which has created segment finished,
>> the segment becomes non-accessible.
>>
>>
>> Test -2 (Postmaster life time)
>> Session - 1
>> -- here it will create segment for postmaster lifetime
>> select dsm_demo_create('this message is from session-1', 1);
>> dsm_demo_create
>> -----------------
>> 827121111
>>
>> Session - 2
>> -----------------
>> select dsm_demo_read(827121111);
>> dsm_demo_read
>> ----------------------------
>> this message is from session-1
>> (1 row)
>>
>>
>> Session-1
>> \q
>>
>> Session-2
>> postgres=# select dsm_demo_read(827121111);
>> dsm_demo_read
>> ---------------
>> this message is from session-1
>> (1 row)
>>
>> Conclusion of Test-2 : a. Segment is accessible for postmaster lifetime.
>> b. if user restart server, segment is
>> not accessible.
>>
>>
>
> Applied dsm_keep_segment_v1.patch and dsm_demo_v1.patch.
> Got the following warning when I tried above example:
>
> postgres=# select dsm_demo_create('this message is from session-new', 1);
> WARNING: dynamic shared memory leak: segment 1402373971 still referenced
> WARNING: dynamic shared memory leak: segment 1402373971 still referenced
> dsm_demo_create
> -----------------
> 1402373971
> (1 row)
>
>

I see that PrintDSMLeakWarning() which emits this warning is for debugging.

--
Amit

On Mon, Jan 27, 2014 at 7:48 PM, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I have extended test (contrib) module dsm_demo such that now user
>> can specify during dsm_demo_create the lifespan of segment.
>
> Applied dsm_keep_segment_v1.patch and dsm_demo_v1.patch.
> Got the following warning when I tried above example:
>
> postgres=# select dsm_demo_create('this message is from session-new', 1);
> WARNING: dynamic shared memory leak: segment 1402373971 still referenced
> WARNING: dynamic shared memory leak: segment 1402373971 still referenced
> dsm_demo_create
> -----------------
> 1402373971
> (1 row)

Thanks for verification.
The reason is that resource owner has reference to segment till commit time
which leads to this warning. The fix is to remove reference from
resource owner when user calls this new API dsm_keep_segment, similar
to what currently dsm_keep_mapping does.

Find the updated patch to fix this problem attached with this mail.

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

On Tue, Jan 28, 2014 at 1:41 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Jan 27, 2014 at 7:48 PM, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> I have extended test (contrib) module dsm_demo such that now user
>>> can specify during dsm_demo_create the lifespan of segment.
>>
>> Applied dsm_keep_segment_v1.patch and dsm_demo_v1.patch.
>> Got the following warning when I tried above example:
>>
>> postgres=# select dsm_demo_create('this message is from session-new', 1);
>> WARNING: dynamic shared memory leak: segment 1402373971 still referenced
>> WARNING: dynamic shared memory leak: segment 1402373971 still referenced
>> dsm_demo_create
>> -----------------
>> 1402373971
>> (1 row)
>
> Thanks for verification.
> The reason is that resource owner has reference to segment till commit time
> which leads to this warning. The fix is to remove reference from
> resource owner when user calls this new API dsm_keep_segment, similar
> to what currently dsm_keep_mapping does.
>
> Find the updated patch to fix this problem attached with this mail.
>

Thanks, the warnings are gone.

--
Amit

Hello,

> Currently there is no way user can keep the dsm
> segments if he wants for postmaster lifetime, so I
> have exposed a new API dsm_keep_segment()
> to implement the same.

I had a short look on this patch.

- DSM implimentation seems divided into generic part (dsm.c) and
platform dependent part(dsm_impl.c). This dsm_keep_segment
puts WIN32 specific part directly into dms.c. I suppose it'd
be better defining DSM_OP_KEEP_SEGMENT and calling dms_impl_op
from dms_keep_segment, or something.

- Repeated calling of dsm_keep_segment even from different
backends creates new (orphan) handles as many as it is called.
Simplly invoking this function in some of extensions intending
to stick segments might results in so many orphan
handles. Something to curb that situation would be needed.

- "Global/PostgreSQL.%u" is the same literal constant with that
occurred in dsm_impl_windows. It should be defined as a
constant (or a macro).

- dms_impl_windows uses errcode_for_dynamic_shared_memory() for
ereport and it finally falls down to
errcode_for_file_access(). I think it is preferable, maybe.

> The specs and need for this API is already discussed
> in thread:
> http://www.postgresql.org/message-id/CA+TgmoaKoGuJQbEdGeYKYSXud9EAidqx77J2_HXzRgFo3Hr46A@mail.gmail.com
>
> I had used dsm_demo (hacked it a bit) module used
> during initial tests for dsm API's to verify the working on
> Windows. So one idea could be that I can extend
> that module to use this new API, so that it can be tested
> by others as well or if you have any other better way, please
> do let me know.

I'll run on windows sooner:-)

> As the discussion about its specs and need is already
> discussed in above mentioned thread, so I will upload
> this patch to CF unless there is any Objection.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jan 28, 2014 at 6:12 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I had a short look on this patch.
>
> - DSM implimentation seems divided into generic part (dsm.c) and
> platform dependent part(dsm_impl.c). This dsm_keep_segment
> puts WIN32 specific part directly into dms.c. I suppose it'd
> be better defining DSM_OP_KEEP_SEGMENT and calling dms_impl_op
> from dms_keep_segment, or something.

That might not be a very good fit, but maybe there should be a
separate function exposed by dsm_impl.c to do the
implementation-dependent part of the work; it would be a no-op except
on Windows.

> - Repeated calling of dsm_keep_segment even from different
> backends creates new (orphan) handles as many as it is called.
> Simplly invoking this function in some of extensions intending
> to stick segments might results in so many orphan
> handles. Something to curb that situation would be needed.

I don't really think this is a problem. Let's just document that
dsm_keep_segment() should not be called more than once per segment.

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

On Tue, Jan 28, 2014 at 4:42 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
>> Currently there is no way user can keep the dsm
>> segments if he wants for postmaster lifetime, so I
>> have exposed a new API dsm_keep_segment()
>> to implement the same.
>
> I had a short look on this patch.

Thanks.

> - DSM implimentation seems divided into generic part (dsm.c) and
> platform dependent part(dsm_impl.c). This dsm_keep_segment
> puts WIN32 specific part directly into dms.c. I suppose it'd
> be better defining DSM_OP_KEEP_SEGMENT and calling dms_impl_op
> from dms_keep_segment, or something.
>
> - Repeated calling of dsm_keep_segment even from different
> backends creates new (orphan) handles as many as it is called.
> Simplly invoking this function in some of extensions intending
> to stick segments might results in so many orphan
> handles. Something to curb that situation would be needed.

I think the right way to fix above 2 comments is as suggested by Robert.

> - "Global/PostgreSQL.%u" is the same literal constant with that
> occurred in dsm_impl_windows. It should be defined as a
> constant (or a macro).
>
> - dms_impl_windows uses errcode_for_dynamic_shared_memory() for
> ereport and it finally falls down to
> errcode_for_file_access(). I think it is preferable, maybe.

Okay, will take care of these in new version after your verification
on Windows.

>> The specs and need for this API is already discussed
>> in thread:
>> http://www.postgresql.org/message-id/CA+TgmoaKoGuJQbEdGeYKYSXud9EAidqx77J2_HXzRgFo3Hr46A@mail.gmail.com
>>
>> I had used dsm_demo (hacked it a bit) module used
>> during initial tests for dsm API's to verify the working on
>> Windows. So one idea could be that I can extend
>> that module to use this new API, so that it can be tested
>> by others as well or if you have any other better way, please
>> do let me know.
>
> I'll run on windows sooner:-)

Please update me once the verification is done on windows.

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

Hello, I've managed to reconstruct windows build environment and
tried to run the previous patch.

====================
> > - DSM implimentation seems divided into generic part (dsm.c) and
> > platform dependent part(dsm_impl.c). This dsm_keep_segment
> > puts WIN32 specific part directly into dms.c. I suppose it'd
> > be better defining DSM_OP_KEEP_SEGMENT and calling dms_impl_op
> > from dms_keep_segment, or something.
> >
> > - Repeated calling of dsm_keep_segment even from different
> > backends creates new (orphan) handles as many as it is called.
> > Simplly invoking this function in some of extensions intending
> > to stick segments might results in so many orphan
> > handles. Something to curb that situation would be needed.
>
> I think the right way to fix above 2 comments is as suggested by Robert.

Fine. I have no objection on that way.

> > - "Global/PostgreSQL.%u" is the same literal constant with that
> > occurred in dsm_impl_windows. It should be defined as a
> > constant (or a macro).
> >
> > - dms_impl_windows uses errcode_for_dynamic_shared_memory() for
> > ereport and it finally falls down to
> > errcode_for_file_access(). I think it is preferable, maybe.
>
> Okay, will take care of these in new version after your verification
> on Windows.

I will apologize in advance for probably silly questions but I
have two problems.

====
Server was crashed by dsm_demo_read on my test environment but
unfortunately the cause is still uncertain for me.

| LOG: server process (PID 19440) was terminated by exception 0xC0000005
| DETAIL: Failed process was running: select dsm_demo_read(4294967297);
| HINT: See C include file "ntstatus.h" for a description of the hexadecimal value.
| LOG: terminating any other active server processes

0xC0000005 is ACCESS_VIOLATION. The crash occurred at aset.c:853

| /* Try to allocate it */
| block = (AllocBlock) malloc(blksize);

Where blksize is 55011304... It's sasier to investigate still
more if I could step into functions in the dynamic loaded module,
but VC2008 IDE skips over the function body:-( Do you have any
idea how I can step into there? My environment is VC2008 Express/
Win7-64. Step-into bounces back at the line where function
definition.

| PG_FUNCTION_INFO_V1(dsm_demo_create);

In contrast, dsm_demo_create doesn't crash and seems to return
sane vaule.

| =# select dsm_demo_create('hoge', 100);
| dsm_demo_create
| -----------------
| 4294967297

===
And the another problem is perhaps not the issue of this module
but it happened for me.

| =# create extension dsm_demo;
| ERROR: could not find function "dsm_demo_create" in file "c:/pgsql/lib/dsm_demo.dll"

I've found that generated dll file exports the function with the
names following after some investigation.

| ...\Debug>dumpbin /EXPORTS dsm_demo.dll
| Microsoft (R) COFF/PE Dumper Version 9.00.21022.08
| Copyright (C) Microsoft Corporation. All rights reserved.
(snipped)
| 1 0 0001100A Pg_magic_func = @ILT+5(_Pg_magic_func)
| 2 1 00011118 pg_finfo_dsm_demo_create = @ILT+275(_pg_finfo_dsm_demo
| _create)
| 3 2 000110AF pg_finfo_dsm_demo_read = @ILT+170(_pg_finfo_dsm_demo_r

Then explicitly designating the function name in 'CREATE
FUNCTION' in dsm_demo--1.0.sql stops the complaint. I might did
something wrong in setting up build environment for this dll but
I have no idea which would cause this kind of trouble..

| CREATE FUNCTION dsm_demo_create(pg_catalog.text, pg_catalog.int4 default 0)
| RETURNS pg_catalog.int8 STRICT
| AS 'MODULE_PATHNAME', 'pg_finfo_dsm_demo_create'
| LANGUAGE C;

Do you have any idea for this?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jan 31, 2014 at 1:35 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, I've managed to reconstruct windows build environment and
> tried to run the previous patch.
Thanks.
>
>
> I will apologize in advance for probably silly questions but I
> have two problems.

I think both the problems are related and the reason is that dsm_demo.dll
is not built properly.
Let us first try to solve your second problem, because I think if
that is solved, you will not face problem-1.

> ====
> Server was crashed by dsm_demo_read on my test environment but
> unfortunately the cause is still uncertain for me.
>
> | LOG: server process (PID 19440) was terminated by exception 0xC0000005
> | DETAIL: Failed process was running: select dsm_demo_read(4294967297);
> | HINT: See C include file "ntstatus.h" for a description of the hexadecimal value.
> | LOG: terminating any other active server processes
>
> 0xC0000005 is ACCESS_VIOLATION. The crash occurred at aset.c:853
>
> | /* Try to allocate it */
> | block = (AllocBlock) malloc(blksize);
>
> Where blksize is 55011304... It's sasier to investigate still
> more if I could step into functions in the dynamic loaded module,
> but VC2008 IDE skips over the function body:-( Do you have any
> idea how I can step into there?

It is because dsm_demo.dll is not built properly.

My environment is VC2008 Express/
> Win7-64. Step-into bounces back at the line where function
> definition.
>
> | PG_FUNCTION_INFO_V1(dsm_demo_create);
>
>
> In contrast, dsm_demo_create doesn't crash and seems to return
> sane vaule.
>
> | =# select dsm_demo_create('hoge', 100);
> | dsm_demo_create
> | -----------------
> | 4294967297

It should not be success, because valid value for second argument is
0 or 1, but you have passed 100. Again here the reason could be
dsm_demo.dll is not built properly.

> ===
> And the another problem is perhaps not the issue of this module
> but it happened for me.
>
> | =# create extension dsm_demo;
> | ERROR: could not find function "dsm_demo_create" in file "c:/pgsql/lib/dsm_demo.dll"
>
> I've found that generated dll file exports the function with the
> names following after some investigation.
>
> | ...\Debug>dumpbin /EXPORTS dsm_demo.dll
> | Microsoft (R) COFF/PE Dumper Version 9.00.21022.08
> | Copyright (C) Microsoft Corporation. All rights reserved.
> (snipped)
> | 1 0 0001100A Pg_magic_func = @ILT+5(_Pg_magic_func)
> | 2 1 00011118 pg_finfo_dsm_demo_create = @ILT+275(_pg_finfo_dsm_demo
> | _create)
> | 3 2 000110AF pg_finfo_dsm_demo_read = @ILT+170(_pg_finfo_dsm_demo_r

When I did dumpbin, I get following:
ordinal hint RVA name

1 0 00001000 Pg_magic_func = Pg_magic_func
2 1 00001030 dsm_demo_create = dsm_demo_create
3 2 00001230 dsm_demo_read = dsm_demo_read
4 3 00001010 pg_finfo_dsm_demo_create = pg_finfo_dsm_demo_create
5 4 00001020 pg_finfo_dsm_demo_read = pg_finfo_dsm_demo_read

There is a dsm_demo.def file which has below symbols

EXPORTS
Pg_magic_func
dsm_demo_create
dsm_demo_read
pg_finfo_dsm_demo_create
pg_finfo_dsm_demo_read

Could you please once check if you have same exports in your
dsm_demo.def

Unless the above 2 things are not same in your env., it can fail
in multiple ways. Let us first try to see why you are not getting above
symbols.

One more thing, can you please once check if
Debug\Postgres\postgres.def contains symbol dsm_keep_segment.
It might be the case that build of postgres in your m/c doesn't have
dsm_keep_segment and then dsm_demo built based on such
postgres will not be proper. Let me know your finding about this?

> Then explicitly designating the function name in 'CREATE
> FUNCTION' in dsm_demo--1.0.sql stops the complaint.

You should not do that, certainly there is problem in you build

> Do you have any idea for this?

I use Visual studio to build in my environment.
How you are setting up?

Can you once do clean build after applying both(dsm_keep_segment
and dsm_demo) the patches.

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

Hello, Now I got workable dll thanks for your advice.

> I think both the problems are related and the reason is that dsm_demo.dll
> is not built properly.
> Let us first try to solve your second problem, because I think if
> that is solved, you will not face problem-1.

Thank you for kindness. I got the situation after successfully
getting correct dll by using .def file after your advice. cl
needs __declspec(dllexport) in the symbol definitions to reveal
them externally, without using .def file.

PostgreSQL platform(?) seems offering a macro PGDLLEXPORT for
such use. I suppose this should be used in extension module dlls
to expose symbols, like this,

- void _PG_init(void);
- Datum dsm_demo_create(PG_FUNCTION_ARGS);
- Datum dsm_demo_read(PG_FUNCTION_ARGS);
===
+ PGDLLEXPORT void _PG_init(void);
+ PGDLLEXPORT Datum dsm_demo_create(PG_FUNCTION_ARGS);
+ PGDLLEXPORT Datum dsm_demo_read(PG_FUNCTION_ARGS);

# This hardly seems to be used commonly...

I followed this instruction to make build environemnt,

http://blog.2ndquadrant.com/compiling-postgresql-extensions-visual-studio-windows/

And the change above enables us to build this module without .def file.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Feb 4, 2014 at 12:25 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, Now I got workable dll thanks for your advice.
>
>> I think both the problems are related and the reason is that dsm_demo.dll
>> is not built properly.
>> Let us first try to solve your second problem, because I think if
>> that is solved, you will not face problem-1.
>
> Thank you for kindness. I got the situation after successfully
> getting correct dll by using .def file after your advice. cl
> needs __declspec(dllexport) in the symbol definitions to reveal
> them externally, without using .def file.
>
> PostgreSQL platform(?) seems offering a macro PGDLLEXPORT for
> such use. I suppose this should be used in extension module dlls
> to expose symbols, like this,
>
> - void _PG_init(void);
> - Datum dsm_demo_create(PG_FUNCTION_ARGS);
> - Datum dsm_demo_read(PG_FUNCTION_ARGS);
> ===
> + PGDLLEXPORT void _PG_init(void);
> + PGDLLEXPORT Datum dsm_demo_create(PG_FUNCTION_ARGS);
> + PGDLLEXPORT Datum dsm_demo_read(PG_FUNCTION_ARGS);
>
> # This hardly seems to be used commonly...

Yeah, for functions we mainly believe to export using .def file
only and so is the case for this module.
Anyway this is just a test module so if things works for you by
changing the above way, its fine. However I wonder why its not
generating .def file for you.
>
> I followed this instruction to make build environemnt,
>
> http://blog.2ndquadrant.com/compiling-postgresql-extensions-visual-studio-windows/
>
> And the change above enables us to build this module without .def file.

Okay, you can complete your test in the way with which you are able to
successfully build it.

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

Hello, I've understood how this works and seems working as
expected.

> Anyway this is just a test module so if things works for you by
> changing the above way, its fine. However I wonder why its not
> generating .def file for you.

Surely.

Getting back on topic, using dsm_keep_segment, I saw postmaster
keeping the section handle for the dsm segment and any session
ever after the creating session gone off could recall the
segment, unlike dsm_keep_mapping couldn't retain them after end
of the creating session. And this exactly resembles linux version
in behavior including attach failure.

The orphan section handles on postmaster have become a matter of
documentation.

Besides all above, I'd like to see a comment for the win32 code
about the 'DuplicateHandle hack', specifically, description that
the DuplicateHandle pushes the copy of the section handle to the
postmaster so the section can retain for the postmaster lifetime.

By the way I have one additional comment.

All postgres processes already keep a section handle for
'Global/PostgreSQL:<pgdata file path>' aside from dsm. It tells
itself is for the file. On the other hand the names for the dsm
sections are 'Global/PostgreSQL.%d'. This seems a bit unkindly as
they don't tell what they are of. Do you mind changing it to,
say, 'Global/PostgreSQL.dsm.%d' or something like that?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, let me say in passing,

> However I wonder why its not generating .def file for you.

Is the 'it' is Visual Studio IDE or CL? Mmm, as far as I know
.def file is a stuff that programmers should write by their hands
as a matter of course.

I've found no way to automatically generate .def files.. (However
itself would be no matter.)

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Feb 6, 2014 at 4:10 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, let me say in passing,
>
>> However I wonder why its not generating .def file for you.
>
> Is the 'it' is Visual Studio IDE or CL? Mmm, as far as I know
> .def file is a stuff that programmers should write by their hands
> as a matter of course.

Using MSVC.
We have gendef.pl which can do it.

Example in Postgres project properties, in
Configuration Properties->Build Events->Pre-Link Event, there
is a Command Line like below which can do the required work.
perl src\tools\msvc\gendef.pl Debug\postgres x64

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

On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, I've understood how this works and seems working as
> expected.
>
>> Anyway this is just a test module so if things works for you by
>> changing the above way, its fine. However I wonder why its not
>> generating .def file for you.
>
> Surely.
>
> Getting back on topic, using dsm_keep_segment, I saw postmaster
> keeping the section handle for the dsm segment and any session
> ever after the creating session gone off could recall the
> segment, unlike dsm_keep_mapping couldn't retain them after end
> of the creating session. And this exactly resembles linux version
> in behavior including attach failure.
>
> The orphan section handles on postmaster have become a matter of
> documentation.
>
> Besides all above, I'd like to see a comment for the win32 code
> about the 'DuplicateHandle hack', specifically, description that
> the DuplicateHandle pushes the copy of the section handle to the
> postmaster so the section can retain for the postmaster lifetime.

Thanks. I shall handle all comments raised by you and send
an updated patch tomorrow.

> By the way I have one additional comment.
>
> All postgres processes already keep a section handle for
> 'Global/PostgreSQL:<pgdata file path>' aside from dsm. It tells
> itself is for the file. On the other hand the names for the dsm
> sections are 'Global/PostgreSQL.%d'. This seems a bit unkindly as
> they don't tell what they are of. Do you mind changing it to,
> say, 'Global/PostgreSQL.dsm.%d' or something like that?

I am not sure if there is any benefit of changing the name as it
is used for identification of segment internally and it is uniquely
identified by segment identifier (segment handle), also I think
something similar is use for posix implementation in
dsm_impl_posix(), so we might need to change that as well.

Also as this name is not introduced by this patch, so I think
it will better to keep this as note to committer for this patch and
if there is an agreement for changing it, I will update the patch.
Whats your opinion?

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

> On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Hello, I've understood how this works and seems working as
>> expected.
>>
>>
>> The orphan section handles on postmaster have become a matter of
>> documentation.

I had explained this in function header of dsm_keep_segment().

>> Besides all above, I'd like to see a comment for the win32 code
>> about the 'DuplicateHandle hack', specifically, description that
>> the DuplicateHandle pushes the copy of the section handle to the
>> postmaster so the section can retain for the postmaster lifetime.

I had added a new function in dsm_impl.c for platform specific
handling and explained about new behaviour in function header.

> - "Global/PostgreSQL.%u" is the same literal constant with that
> occurred in dsm_impl_windows. It should be defined as a
> constant (or a macro).

Changed to hash define.

> - dms_impl_windows uses errcode_for_dynamic_shared_memory() for
> ereport and it finally falls down to
> errcode_for_file_access(). I think it is preferable, maybe

Changed as per suggestion.

Please find new version of patch attached with this mail containing
above changes.

Thanks for review.

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

Hello,

> Please find new version of patch attached with this mail containing
> above changes.

This patch applies cleanly on current HEAD and build completed
successfully on both Windows and Linux. (but master needed to be
rewinded to some time ago for some compile errors.)

This works correctly as before.

Finally before send to commiters, would you mind changing the
name of the segment "Global/PostgreSQL.%u" as
'Global/PostgreSQL.dsm.%u" or something mentioned in the last my
email? However, it is a bit different thing from this patch so
I have no intention to compel to do the changing.

> >> The orphan section handles on postmaster have become a matter of
> >> documentation.
>
> I had explained this in function header of dsm_keep_segment().

The comment satisfies me. Thank you.

> I had added a new function in dsm_impl.c for platform specific
> handling and explained about new behaviour in function header.

This seems quite clear for me.

> > - "Global/PostgreSQL.%u" is the same literal constant with that
> > occurred in dsm_impl_windows. It should be defined as a
> > constant (or a macro).
>
> Changed to hash define.

Thank you.

> > - dms_impl_windows uses errcode_for_dynamic_shared_memory() for
> > ereport and it finally falls down to
> > errcode_for_file_access(). I think it is preferable, maybe
>
> Changed as per suggestion.

I saw it done.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Feb 12, 2014 at 2:02 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
>> Please find new version of patch attached with this mail containing
>> above changes.
>
> This patch applies cleanly on current HEAD and build completed
> successfully on both Windows and Linux. (but master needed to be
> rewinded to some time ago for some compile errors.)
>
> This works correctly as before.
>
> Finally before send to commiters, would you mind changing the
> name of the segment "Global/PostgreSQL.%u" as
> 'Global/PostgreSQL.dsm.%u" or something mentioned in the last my
> email?

Actually in that case, to maintain consistency we need to change even in
function dsm_impl_posix() which uses segment name as "/PostgreSQL.%u".
For windows, we have added "Global" to "/PostgreSQL.%u", as it is
mandate by windows spec.
To be honest, I see no harm in changing the name as per your suggestion,
as it can improve segment naming for dynamic shared memory segments,
however there is no clear problem with current name as well, so I don't
want to change in places this patch has no relation.

I think best thing to do here is to put it as Notes To Committer, something
like:
Some suggestions for Committer to consider:
"Change the name of dsm segments from .. to .."

In general, what I see is that they consider all discussion in thread, but
putting some special notes like above will reduce the chance of getting
overlooked by them. I have done as a reviewer previously and it worked
well.

> However, it is a bit different thing from this patch so
> I have no intention to compel to do the changing.

Thanks to you for understanding my position.

Thanks for reviewing the patch so carefully, especially Windows part
which I think was bit tricky for you to setup.

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

Hello, I've marked this patch as 'Ready for committer'.

> To be honest, I see no harm in changing the name as per your suggestion,
> as it can improve segment naming for dynamic shared memory segments,
> however there is no clear problem with current name as well, so I don't
> want to change in places this patch has no relation.

Okay, let's go with it as it is.

> I think best thing to do here is to put it as Notes To Committer, something
> like:
> Some suggestions for Committer to consider:
> "Change the name of dsm segments from .. to .."

> In general, what I see is that they consider all discussion in thread, but
> putting some special notes like above will reduce the chance of getting
> overlooked by them. I have done as a reviewer previously and it worked
> well.

Thank you for the sugestion.

> > However, it is a bit different thing from this patch so
> > I have no intention to compel to do the changing.
>
> Thanks to you for understanding my position.
>
> Thanks for reviewing the patch so carefully, especially Windows part
> which I think was bit tricky for you to setup.

It's my presure and I learned a lot.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for letting me know of that.

> Using MSVC.
> We have gendef.pl which can do it.

Mmm.. My eyes skipped over it. Everything became clear for
me. Thank you.

> Example in Postgres project properties, in
> Configuration Properties->Build Events->Pre-Link Event, there
> is a Command Line like below which can do the required work.
> perl src\tools\msvc\gendef.pl Debug\postgres x64

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Sat, Feb 8, 2014 at 2:31 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> Hello, I've understood how this works and seems working as
>>> expected.
>>>
>>>
>>> The orphan section handles on postmaster have become a matter of
>>> documentation.
>
> I had explained this in function header of dsm_keep_segment().
>
>>> Besides all above, I'd like to see a comment for the win32 code
>>> about the 'DuplicateHandle hack', specifically, description that
>>> the DuplicateHandle pushes the copy of the section handle to the
>>> postmaster so the section can retain for the postmaster lifetime.
>
> I had added a new function in dsm_impl.c for platform specific
> handling and explained about new behaviour in function header.
>
>
>> - "Global/PostgreSQL.%u" is the same literal constant with that
>> occurred in dsm_impl_windows. It should be defined as a
>> constant (or a macro).
>
> Changed to hash define.
>
>> - dms_impl_windows uses errcode_for_dynamic_shared_memory() for
>> ereport and it finally falls down to
>> errcode_for_file_access(). I think it is preferable, maybe
>
> Changed as per suggestion.
>
> Please find new version of patch attached with this mail containing
> above changes.

I took a look at this patch. It seems to me that it doesn't do a very
good job maintaining the abstraction boundary between what the dsm.c
layer knows about and what the dsm_impl.c layer knows about. However,
AFAICS, these problems are purely cosmetic, so I took a crack at
fixing them. I retitled the new implementation-layer function to
dsm_impl_keep_segment(), swapped the order of the arguments for
consistency with other code, adjusted the dsm_impl.c code slightly to
avoid assuming that only the Windows implementation works on Windows
(that's currently true, but we could probably make the mmap
implementation work there as well), and retooled some of the comments
to read better in English. I'm happy with the attached version but
don't have a Windows box to test it there.

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

On Mon, Mar 10, 2014 at 8:18 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I took a look at this patch. It seems to me that it doesn't do a very
> good job maintaining the abstraction boundary between what the dsm.c
> layer knows about and what the dsm_impl.c layer knows about. However,
> AFAICS, these problems are purely cosmetic, so I took a crack at
> fixing them. I retitled the new implementation-layer function to
> dsm_impl_keep_segment(), swapped the order of the arguments for
> consistency with other code, adjusted the dsm_impl.c code slightly to
> avoid assuming that only the Windows implementation works on Windows
> (that's currently true, but we could probably make the mmap
> implementation work there as well), and retooled some of the comments
> to read better in English. I'm happy with the attached version but
> don't have a Windows box to test it there.

Thank you for looking into patch. I have verified that attached patch
works fine on Windows.

One observation in new version of patch:

+ {
+ char name[64];
+
+ snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+ _dosmaperr(GetLastError());
+ ereport(ERROR,
+ (errcode_for_dynamic_shared_memory(),
+ errmsg("could not duplicate handle: %m")));
+ }

Now, the patch doesn't use segment *name* in errmsg which makes
it and handle passed in function dsm_impl_keep_segment() redundant.
I think its better to use name in errmsg as it is used at other places
in code as well and make the error more meaningful. However if you
feel it is better otherwise, then we should remove not required variable
and parameter in function dsm_impl_keep_segment()

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

On Mon, Mar 10, 2014 at 9:48 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Mar 10, 2014 at 8:18 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I took a look at this patch. It seems to me that it doesn't do a very
>> good job maintaining the abstraction boundary between what the dsm.c
>> layer knows about and what the dsm_impl.c layer knows about. However,
>> AFAICS, these problems are purely cosmetic, so I took a crack at
>> fixing them. I retitled the new implementation-layer function to
>> dsm_impl_keep_segment(), swapped the order of the arguments for
>> consistency with other code, adjusted the dsm_impl.c code slightly to
>> avoid assuming that only the Windows implementation works on Windows
>> (that's currently true, but we could probably make the mmap
>> implementation work there as well), and retooled some of the comments
>> to read better in English. I'm happy with the attached version but
>> don't have a Windows box to test it there.
>
> Thank you for looking into patch. I have verified that attached patch
> works fine on Windows.
>
> One observation in new version of patch:
>
> + {
> + char name[64];
> +
> + snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
> + _dosmaperr(GetLastError());
> + ereport(ERROR,
> + (errcode_for_dynamic_shared_memory(),
> + errmsg("could not duplicate handle: %m")));
> + }

I have updated the patch to change message as below:
errmsg("could not duplicate handle for \"%s\": %m",
name)));

Let me know your suggestions?

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

On Mon, Mar 10, 2014 at 12:44 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Mar 10, 2014 at 9:48 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Mon, Mar 10, 2014 at 8:18 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I took a look at this patch. It seems to me that it doesn't do a very
>>> good job maintaining the abstraction boundary between what the dsm.c
>>> layer knows about and what the dsm_impl.c layer knows about. However,
>>> AFAICS, these problems are purely cosmetic, so I took a crack at
>>> fixing them. I retitled the new implementation-layer function to
>>> dsm_impl_keep_segment(), swapped the order of the arguments for
>>> consistency with other code, adjusted the dsm_impl.c code slightly to
>>> avoid assuming that only the Windows implementation works on Windows
>>> (that's currently true, but we could probably make the mmap
>>> implementation work there as well), and retooled some of the comments
>>> to read better in English. I'm happy with the attached version but
>>> don't have a Windows box to test it there.
>>
>> Thank you for looking into patch. I have verified that attached patch
>> works fine on Windows.
>>
>> One observation in new version of patch:
>>
>> + {
>> + char name[64];
>> +
>> + snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
>> + _dosmaperr(GetLastError());
>> + ereport(ERROR,
>> + (errcode_for_dynamic_shared_memory(),
>> + errmsg("could not duplicate handle: %m")));
>> + }
>
> I have updated the patch to change message as below:
> errmsg("could not duplicate handle for \"%s\": %m",
> name)));
>
> Let me know your suggestions?

Looks good, committed. However, I changed it so that
dsm_keep_segment() does not also perform the equivalent of
dsm_keep_mapping(); those are two separate operations.

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

On Mon, Mar 10, 2014 at 11:37 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Looks good, committed. However, I changed it so that
> dsm_keep_segment() does not also perform the equivalent of
> dsm_keep_mapping(); those are two separate operations.

So are you expecting that if some one needs to retain dynamic segment's
till PM lifetime, they should call both dsm_keep_segment() and
dsm_keep_mapping()?

If we don't call both, it can lead to following warning:
postgres=# select dsm_demo_create('this message is from session-new', 1);
WARNING: dynamic shared memory leak: segment 1402373971 still referenced

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

On Mon, Mar 10, 2014 at 11:26 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Mar 10, 2014 at 11:37 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Looks good, committed. However, I changed it so that
>> dsm_keep_segment() does not also perform the equivalent of
>> dsm_keep_mapping(); those are two separate operations.
>
> So are you expecting that if some one needs to retain dynamic segment's
> till PM lifetime, they should call both dsm_keep_segment() and
> dsm_keep_mapping()?

If they want to keep both the mapping and the segment, yes. But in
general those two things are independent of each other. A process
could want to map the segment and store some data in it, and then it
could want to unmap the segment; and then later the segment could be
mapped again (perhaps from some other backend) to get the data out.

> If we don't call both, it can lead to following warning:
> postgres=# select dsm_demo_create('this message is from session-new', 1);
> WARNING: dynamic shared memory leak: segment 1402373971 still referenced

Well, that's just an artifact of the coding of dsm_demo_create().
Code that doesn't use dsm_keep_mapping() needs to be sure to call
dsm_detach() in the non-error path.

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