Retain dynamic shared memory segments for postmaster lifetime

On Sun, Jan 12, 2014 at 12:41 PM, Amit Kapila <amit.kapila16@gmail.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:
/messages/by-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.kapila16@gmail.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

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

On Mon, Jan 27, 2014 at 11:18 PM, Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit.kapila16@gmail.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

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

On Mon, Jan 27, 2014 at 7:48 PM, Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit.kapila16@gmail.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.kapila16@gmail.com> wrote:

On Mon, Jan 27, 2014 at 7:48 PM, Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jan 13, 2014 at 2:50 PM, Amit Kapila <amit.kapila16@gmail.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

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

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:
/messages/by-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

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

On Tue, Jan 28, 2014 at 6:12 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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

On Tue, Jan 28, 2014 at 4:42 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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:
/messages/by-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

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

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

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

On Fri, Jan 31, 2014 at 1:35 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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

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

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

On Tue, Feb 4, 2014 at 12:25 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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

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

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

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

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

On Thu, Feb 6, 2014 at 4:10 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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

On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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

On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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

On Wed, Feb 12, 2014 at 2:02 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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

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