Re: dynamic shared memory

On Fri, Sep 20, 2013 at 5:14 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
>
> On 2013-09-19 11:44:34 -0400, Robert Haas wrote:
>> On Wed, Sep 18, 2013 at 1:42 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
>> >> --- /dev/null
>> >> +++ b/src/backend/storage/ipc/dsm.c
>> >> +#define PG_DYNSHMEM_STATE_FILE PG_DYNSHMEM_DIR "/state"
>> >> +#define PG_DYNSHMEM_NEW_STATE_FILE PG_DYNSHMEM_DIR "/state.new"
>> >
>> > Hm, I guess you dont't want to add it to global/ or so because of the
>> > mmap implementation where you presumably scan the directory?
>>
>> Yes, and also because I thought this way would make it easier to teach
>> things like pg_basebackup (or anybody's home-brew scripts) to just
>> skip that directory completely. Actually, I was wondering if we ought
>> to have a directory under pgdata whose explicit charter it was to
>> contain files that shouldn't be copied as part of a base backup.
>> pg_do_not_back_this_up.
>
> Wondered exactly about that as soon as you've mentioned
> pg_basebackup. pg_local/?
>
>> >> + /* Determine size for new control segment. */
>> >> + maxitems = PG_DYNSHMEM_FIXED_SLOTS
>> >> + + PG_DYNSHMEM_SLOTS_PER_BACKEND * MaxBackends;
>> >
>> > It seems likely that MaxConnections would be sufficient?
>>
>> I think we could argue about the best way to set this until the cows
>> come home, but I don't think it probably matters much at this point.
>> We can always change the formula later as we gain experience.
>> However, I don't have a principled reason for assuming that only
>> user-connected backends will create dynamic shared memory segments.
>
> Hm, yes. I had MaxBackends down as MaxConnections + autovacuum stuff;
> but max_worker_processes are in there now, so you're right that doesn't
> make sense.
>
>> >> +/*
>> >> + * Read and parse the state file.
>> >> + *
>> >
>> > Perhaps CRC32 the content?
>>
>> I don't see the point. If the file contents are garbage that happens
>> to look like a number, we'll go "oh, there isn't any such segment" or
>> "oh, there is such a segment but it doesn't look like a control
>> segment, so forget it". There are a lot of things we really ought to
>> be CRCing to avoid corruption risk, but I can't see how this is
>> remotely one of them.
>
> I was worried about a partially written file or containing contents from
> two different postmaster cycles, but it's actually far too small for
> that...
> I initially had thought you'd write the contents of the entire shared
> control segment there, not just it's id.
>
>> >> + /* Create or truncate the file. */
>> >> + statefd = open(PG_DYNSHMEM_NEW_STATE_FILE, O_RDWR|O_CREAT|O_TRUNC, 0600);
>> >
>> > Doesn't this need a | PG_BINARY?
>>
>> It's a text file. Do we need PG_BINARY anyway?
>
> I'd say yes. Non binary mode stuff on windows does stuff like
> transforming LF <=> CRLF on reading/writing, which makes sizes not match
> up and similar ugliness.
> Imo there's little reason to use non-binary mode for anything written
> for postgres' own consumption.

On checking about this in code, I found the below comment which
suggests LF<=> CRLF is not an issue (in windows it uses pgwin32_open
to open a file):

/*

* NOTE: this is also used for opening text files.

* WIN32 treats Control-Z as EOF in files opened in text mode.

* Therefore, we open files in binary mode on Win32 so we can read

* literal control-Z. The other affect is that we see CRLF, but

* that is OK because we can already handle those cleanly.

*/

Second instance, I noticed in code as below which again suggests CRLF
should not be an issue until file mode is specifically set to TEXT
mode which is not the case with current usage of open in dynamic
shared memory code.

#ifdef WIN32

/* use CRLF line endings on Windows */

_setmode(_fileno(fh), _O_TEXT);

#endif

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