Обсуждение: 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.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: > 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.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 referenceddsm_demo_create ----------------- 1402373971 (1 row) -- Amit
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
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
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 andcalling 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.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
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: >> 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.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
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.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
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.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
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
> 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
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
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.kapila16@gmail.com> wrote: >> 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. 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@gmail.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.kapila16@gmail.com> wrote: > On Mon, Mar 10, 2014 at 8:18 PM, Robert Haas <robertmhaas@gmail.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.kapila16@gmail.com> wrote: > On Mon, Mar 10, 2014 at 9:48 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Mon, Mar 10, 2014 at 8:18 PM, Robert Haas <robertmhaas@gmail.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@gmail.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.kapila16@gmail.com> wrote: > On Mon, Mar 10, 2014 at 11:37 PM, Robert Haas <robertmhaas@gmail.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