RE: [PoC] pg_upgrade: allow to upgrade publisher node
От | Hayato Kuroda (Fujitsu) |
---|---|
Тема | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Дата | |
Msg-id | TYAPR01MB5866D4AAD5B77E276911C6C8F5EFA@TYAPR01MB5866.jpnprd01.prod.outlook.com обсуждение исходный текст |
Ответ на | Re: [PoC] pg_upgrade: allow to upgrade publisher node (Peter Smith <smithpb2250@gmail.com>) |
Список | pgsql-hackers |
Dear Peter, Thank you for reviewing! > > ====== > src/bin/pg_upgrade/controldata.c > > 1. get_control_data > > + if (GET_MAJOR_VERSION(cluster->major_version) >= 1700) > + { > + bool have_error = false; > + > + p = strchr(p, ':'); > + > + if (p == NULL || strlen(p) <= 1) > + pg_fatal("%d: controldata retrieval problem", __LINE__); > + > + p++; /* remove ':' char */ > + > + p = strpbrk(p, "01234567890ABCDEF"); > + > + if (p == NULL || strlen(p) <= 1) > + pg_fatal("%d: controldata retrieval problem", __LINE__); > + > + cluster->controldata.chkpnt_latest = > + strtoLSN(p, &have_error); > > 1a. > The declaration assignment of 'have_error' is redundant because it > gets overwritten before it is checked anyhow. > > ~ > > 1b. > IMO that first check logic should also be shifted to be *inside* the > strtoLSN and it would just return have_error true. This eliminates > having 2x pg_fatal that have the same purpose. > > ~~~ > > 2. strtoLSN > > +/* > + * Convert String to XLogRecPtr. > + * > + * This function is ported from pg_lsn_in_internal(). The function cannot be > + * called from client binaries. > + */ > +XLogRecPtr > +strtoLSN(const char *str, bool *have_error) > > SUGGESTION (comment wording) > This function is ported from pg_lsn_in_internal() which cannot be > called from client binaries. These changes are reverted. > src/bin/pg_upgrade/function.c > > 3. struct plugin_list > > +typedef struct plugin_list > +{ > + int dbnum; > + char *plugin; > + struct plugin_list *next; > +} plugin_list; > > I found that name confusing. IMO should be like 'plugin_list_elem'. > > e.g. it gets too strange in subsequent code: > + plugin_list *newentry = (plugin_list *) pg_malloc(sizeof(plugin_list)); > > ~~~ > > 4. is_plugin_unique > > +/* Has the given plugin already been listed? */ > +static bool > +is_plugin_unique(plugin_list_head *listhead, const char *plugin) > +{ > + plugin_list *point; > + > + /* Quick return if the head is NULL */ > + if (listhead == NULL) > + return true; > + > + /* Seek the plugin list */ > + for (point = listhead->head; point; point = point->next) > + { > + if (strcmp(point->plugin, plugin) == 0) > + return false; > + } > + > + return true; > +} > > What's the meaning of the name 'point'? Maybe something generic like > 'cur' or similar is better? > > ~~~ > > 5. get_output_plugins > > +/* > + * Load the list of unique output plugins. > + * > + * XXX: Currently, we extract the list of unique output plugins, but this may > + * be overkill. The list is used for two purposes - 1) to allocate the minimal > + * memory for the library list and 2) to skip storing duplicated plugin names. > + * However, the consumer check_loadable_libraries() can avoid double checks > for > + * the same library. The above means that we can arrange output plugins without > + * considering their uniqueness, so that we can remove this function. > + */ > +static plugin_list_head * > +get_output_plugins(void) > +{ > + plugin_list_head *head = NULL; > + int dbnum; > + > + /* Quick return if there are no logical slots to be migrated. */ > + if (count_old_cluster_logical_slots() == 0) > + return NULL; > + > + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++) > + { > + LogicalSlotInfoArr *slot_arr = &old_cluster.dbarr.dbs[dbnum].slot_arr; > + int slotnum; > + > + for (slotnum = 0; slotnum < slot_arr->nslots; slotnum++) > + { > + LogicalSlotInfo *slot = &slot_arr->slots[slotnum]; > + > + /* Add to the list if the plugin has not been listed yet */ > + if (is_plugin_unique(head, slot->plugin)) > + add_plugin_list_item(&head, dbnum, slot->plugin); > + } > + } > + > + return head; > +} > > About the XXX. Yeah, since the uniqueness seems checked later anyway > all this extra code seems overkill. Instead of all the extra code you > just need a comment to mention how it will be sorted and checked > later. > > But even if you prefer to keep it, I thought those 2 functions > 'is_plugin_unique()' and 'add_plugin_list_item()' could have been > combined to just have 'add_plugin_list_unique_item()'. Since order > does not matter, such a function would just add items to the end of > the list (after finding uniqueness) instead of to the head. Based on suggestions from you and Hou[1], I withdrew to check their uniqueness. So these functions and structures are removed. > 6. get_loadable_libraries > > FirstNormalObjectId); > + > totaltups += PQntuples(ress[dbnum]); > ~ > > The extra blank line in the existing code is not needed in this patch. Removed. > 7. get_loadable_libraries > > int rowno; > + plugin_list *point; > > ~ > > Same as a prior comment #4. What's the meaning of the name 'point'? The variable was removed. > 8. get_loadable_libraries > + > + /* > + * If the old cluster has logical replication slots, plugins used by > + * them must be also stored. It must be done only once, so do it at > + * dbnum == 0 case. > + */ > + if (output_plugins == NULL) > + continue; > + > + if (dbnum != 0) > + continue; > > This logic seems misplaced. If this "must be done only once" then why > is it within the db loop in the first place? Shouldn't this be done > seperately outside the loop? The logic was removed. > src/bin/pg_upgrade/info.c > > 9. > +/* > + * Helper function for get_old_cluster_logical_slot_infos() > + */ > +static WALAvailability > +GetWALAvailabilityByString(const char *str) > > Should this be forward declared like the other static functions are? The function was removed. > 10. get_old_cluster_logical_slot_infos > > + for (slotnum = 0; slotnum < num_slots; slotnum++) > + { > + LogicalSlotInfo *curr = &slotinfos[slotnum]; > + bool have_error = false; > > Here seems an unnecessary assignment to 'have_error' because it will > always be assigned again before it is checked. The variable was removed. > 11. get_old_cluster_logical_slot_infos > > + curr->confirmed_flush = strtoLSN( > + PQgetvalue(res, > + slotnum, > + i_confirmed_flush), > + &have_error); > + curr->wal_status = GetWALAvailabilityByString( > + PQgetvalue(res, > + slotnum, > + i_wal_status)); > > Can this excessive wrapping be improved? Maybe new vars are needed. The part was removed. > 12. > +static void > +print_slot_infos(LogicalSlotInfoArr *slot_arr) > +{ > + int slotnum; > + > + for (slotnum = 0; slotnum < slot_arr->nslots; slotnum++) > + { > + LogicalSlotInfo *slot_info = &slot_arr->slots[slotnum]; > + > + if (slotnum == 0) > + pg_log(PG_VERBOSE, "Logical replication slots within the database:"); > + > + pg_log(PG_VERBOSE, "slotname: \"%s\", plugin: \"%s\", two_phase: %d", > + slot_info->slotname, > + slot_info->plugin, > + slot_info->two_phase); > + } > +} > > This seems an odd way to output the heading. Isn't it better to put > this outside the loop? > > SUGGESTION > if (slot_arr->nslots > 0) > pg_log(PG_VERBOSE, "Logical replication slots within the database:"); Fixed. > src/bin/pg_upgrade/pg_upgrade.c > > 13. > +/* > + * setup_new_cluster() > + * > + * Starts a new cluster for updating the wal_level in the control fine, then > + * does final setups. Logical slots are also created here. > + */ > +static void > +setup_new_cluster(void) > > typo > > /control fine/control file/ Fixed. [1]: https://www.postgresql.org/message-id/OS0PR01MB57165A8F24BEFF5F4CCBBE5994EFA%40OS0PR01MB5716.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
В списке pgsql-hackers по дате отправления:
Предыдущее
От: "Hayato Kuroda (Fujitsu)"Дата:
Сообщение: RE: [PoC] pg_upgrade: allow to upgrade publisher node
Следующее
От: "Hayato Kuroda (Fujitsu)"Дата:
Сообщение: RE: [PoC] pg_upgrade: allow to upgrade publisher node