On Sun, Sep 12, 2021 at 10:22 PM Michael Paquier <mich...@paquier.xyz>
wrote:
> On Fri, Sep 10, 2021 at 03:56:18PM +0900, Michael Paquier wrote:
> > And this part leads me to the attached, where the addition of the JSON
> > format would result in the addition of a couple of lines.
>
> Okay, I have worked through the first half of the patch set, and
> applied the improved versions of 0001 (refactoring of the chunk
> protocol) and 0002 (addition of the tests for csvlog).
>
Thanks. I finally got a chance to look through those changes. I like it.
The popcount and pulling out the flags are much easier to follow than the
old hard coded letters.
> I have not looked in details at 0003 and 0004 yet. Still, here are
> some comments after a quick scan.
>
> + * elog-internal.h
> I'd rather avoid the hyphen, and use elog_internal.h.
>
> +#define FORMATTED_TS_LEN 128
> +extern char formatted_start_time[FORMATTED_TS_LEN];
> +extern char formatted_log_time[FORMATTED_TS_LEN];
> +
> +void setup_formatted_log_time(void);
> +void setup_formatted_start_time(void);
> We could just use a static buffer in each one of those routines, and
> return back a pointer to the caller.
>
+1
> + else if ((Log_destination & LOG_DESTINATION_JSONLOG) &&
> + (jsonlogFile == NULL ||
> + time_based_rotation || (size_rotation_for &
> LOG_DESTINATION_JSONLOG)))
> [...]
> + /* Write to JSON log if enabled */
> + else if (Log_destination & LOG_DESTINATION_JSONLOG)
> + {
> Those bits in 0004 are wrong. They should be two "if" clauses. This
> is leading to issues when setting log_destination to multiple values
> with jsonlog in the set of values and logging_connector = on, and the
> logs are not getting redirected properly to the .json file. We would
> get the data for the .log and .csv files though. This issue also
> happens with the original patch set applied on top of e757080. I
> think that we should also be more careful with the way we free
> StringInfoData in send_message_to_server_log(), or we are going to
> finish with unwelcome leaks.
>
Good catch. Staring at that piece again, that's tricky as it tries to
aggressively free the buffer before calling write_cvslog(...). Which can't
just be duplicated for additional destinations.
I think we need to pull up the negative case (i.e. syslogger not available)
before the other destinations and if it matches, free and exit early.
Otherwise, free the buffer and call whatever destination routines are
enabled.
> The same coding pattern is now repeated three times in logfile_rotate():
> - Check if a logging type is enabled.
> - Optionally open new file, with logfile_open().
> - Business with ENFILE and EMFILE.
> - pfree() and save of the static FILE* ane file name for each type.
> I think that we have enough material for a wrapper routine that does
> this work, where we pass down LOG_DESTINATION_* and pointers to the
> static FILE* and the static last file names. That would have avoided
> the bug I found above.
>
I started on a bit of this as well. There's so much overlap already between
the syslog_ and csvlog code that I'm going to put that together first. Then
the json addition should just fall into a couple of call sites.
I'm thinking of adding an internal struct to house the FILE* and file
names. Then all the opening, closing, and log rotation code can pass
pointers to those and centralize the pfree() and NULL checks.
> The rebased patch set, that has reworked the tests to be in line with
> HEAD, also fails. They compile.
>
> Sehrope, could you adjust that?
Yes I'm looking to sync those up and address the above later this week.
Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/
