+ /* Build timestamp directory path */
+ len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp);
+
+ if (len >= MAXPGPATH)
+ pg_fatal("directory path for log files, %s/%s, is too long",
+ logdir, timestamp);
```
> In the pg_fatal call, I believe logdir should be log_basedir.
We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir.
> The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is:
```
> errno = save_errno;
> va_copy(ap2, ap);
> required_len = vsnprintf(NULL, 0, fmt, ap2) + 1;
> va_end(ap2);
```
> Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m.
I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where they are calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically as long as errno is not overwritten - which it will now be. Thank you!
>The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional?
I could add a switch-case to print it out. Is that important? What do you think?
I have fixed the rest of your suggestions. Thank you, Chao Li!
> We can get rid of below alignment related changes in unrelated test parts.
They are added when I run pgperltidy. Anyone else trying to change the file after this would see the same thing if we don't change it. Should I move it into another patch?
I have fixed the rest of it. Thank you, Shveta Malik!
01.
Found that pg_log_generic_v() has some prefix but internal_log_file_write() does not.
It means output strings are not the same. For example, on terminal:
```
pg_createsubscriber: error: standby server is running
pg_createsubscriber: hint: Stop the standby server and try again.
```
But on log file:
```
standby server is running
Stop the standby server and try again.
```
It's because pg_log_generic_v() has the format like below. I.e., the program name
is printed at the begining, and some prefix also exists in some cases.
${program name}: {error: |warning: |detail: |hint: } content
I cannot find such a difference on pg_upgrade: no prefix exists in any cases.
So, what should be here? My preference is to basically follow pg_log_generic_v()
But remove the program name. How about others?
I haven't changed the output of pg_log_generic_v() yet. Shall I add the prefix to the output? I have done the rest of your suggestions. Thank you!
Regards,
Gyan Sreejith
